Merge pull request sakaiproject#1787 from buckett/SAK-30364

SAK-30364 Fix the permission issue in SubethaSMTP.

mailarchive/mailarchive-subetha/pom.xml

@@ -70,6 +70,10 @@
             <artifactId>commons-net</artifactId>
             <version>1.1.0</version>
         </dependency>
+        <dependency>
+            <groupId>javax.servlet</groupId>
+            <artifactId>servlet-api</artifactId>
+        </dependency>
         <dependency>
             <groupId>org.mockito</groupId>
             <artifactId>mockito-core</artifactId>

mailarchive/mailarchive-subetha/src/java/org/sakaiproject/mailarchive/SakaiMessageHandlerFactory.java

@@ -20,15 +20,18 @@
 import org.sakaiproject.site.api.SiteService;
 import org.sakaiproject.thread_local.api.ThreadLocalManager;
 import org.sakaiproject.time.api.TimeService;
+import org.sakaiproject.tool.api.*;
 import org.sakaiproject.user.api.User;
 import org.sakaiproject.user.api.UserDirectoryService;
+import org.sakaiproject.user.api.UserNotDefinedException;
 import org.sakaiproject.util.Validator;
 import org.sakaiproject.util.Web;
 import org.subethamail.smtp.MessageContext;
 import org.subethamail.smtp.*;
 import org.subethamail.smtp.server.SMTPServer;
 
 import javax.mail.*;
+import javax.mail.Session;
 import javax.mail.internet.ContentType;
 import javax.mail.internet.InternetAddress;
 import javax.mail.internet.MimeMessage;
@@ -49,7 +52,6 @@
 public class SakaiMessageHandlerFactory implements MessageHandlerFactory {
 
     private Log log = LogFactory.getLog(SakaiMessageHandlerFactory.class);
-    private InternationalizedMessages rb;
 
     /**
      * The user name of the postmaster user - the one who posts incoming mail.
@@ -58,6 +60,7 @@ public class SakaiMessageHandlerFactory implements MessageHandlerFactory {
 
     private SMTPServer server;
 
+    private InternationalizedMessages rb;
     private ServerConfigurationService serverConfigurationService;
     private EntityManager entityManager;
     private AliasService aliasService;
@@ -67,6 +70,7 @@ public class SakaiMessageHandlerFactory implements MessageHandlerFactory {
     private ThreadLocalManager threadLocalManager;
     private ContentHostingService contentHostingService;
     private MailArchiveService mailArchiveService;
+    private SessionManager sessionManager;
 
     public void setInternationalizedMessages(InternationalizedMessages rb) {
         this.rb = rb;
@@ -108,10 +112,26 @@ public void setMailArchiveService(MailArchiveService mailArchiveService) {
         this.mailArchiveService = mailArchiveService;
     }
 
+    public void setSessionManager(SessionManager sessionManager) {
+        this.sessionManager = sessionManager;
+    }
+
     // used when parsing email header parts
     private static final String NAME_PREFIX = "name=";
 
     public void init() {
+        Objects.requireNonNull(rb, "ResourceLoader must be set");
+        Objects.requireNonNull(serverConfigurationService, "ServerConfigurationService must be set");
+        Objects.requireNonNull(entityManager, "EntityManager must be set");
+        Objects.requireNonNull(aliasService, "AliasService must be set");
+        Objects.requireNonNull(userDirectoryService, "UserDirectoryService must be set");
+        Objects.requireNonNull(siteService, "SiteService must be set");
+        Objects.requireNonNull(timeService, "TimeService must be set");
+        Objects.requireNonNull(threadLocalManager, "ThreadLocalManager must be set");
+        Objects.requireNonNull(contentHostingService, "ContentHostingService must be set");
+        Objects.requireNonNull(mailArchiveService, "MailArchiveService must be set");
+        Objects.requireNonNull(sessionManager, "SessionManager must be set");
+
         if (serverConfigurationService.getBoolean("smtp.enabled", false)) {
             server = new SMTPServer(this);
 
@@ -145,7 +165,6 @@ public MessageHandler create(MessageContext ctx) {
             private String from;
             private Collection<Recipient> recipients = new LinkedList<>();
 
-
             @Override
             public void from(String from) throws RejectException {
                 this.from = from;
@@ -178,8 +197,11 @@ public void data(InputStream data) throws RejectException, IOException {
                 // smallBuffer.
                 // SharedFileInputStream fis = new SharedFileInputStream(null);
 
-
+                org.sakaiproject.tool.api.Session session = sessionManager.getCurrentSession();
                 try {
+                    session.setUserId(POSTMASTER);
+                    session.setUserEid(POSTMASTER);
+
                     // TODO Proper properties.
                     // The reads the entire body of the message into a byte array which is far from optimal.
                     MimeMessage msg = new MimeMessage(Session.getDefaultInstance(new Properties()), data);
@@ -310,6 +332,7 @@ public void data(InputStream data) throws RejectException, IOException {
                     // TODO
                     throw new RejectException();
                 } finally {
+                    session.clear();
                     // clear out any current current bindings
                     threadLocalManager.clear();
                 }
@@ -323,8 +346,12 @@ public void data(InputStream data) throws RejectException, IOException {
              */
             protected MailArchiveChannel getMailArchiveChannel(String mailId) throws RejectException {
 
+                org.sakaiproject.tool.api.Session session = sessionManager.getCurrentSession();
                 try {
 
+                    session.setUserId(POSTMASTER);
+                    session.setUserEid(POSTMASTER);
+
                     // eat the no-reply
                     if ("no-reply".equalsIgnoreCase(mailId)) {
                         if (log.isInfoEnabled()) {
@@ -350,17 +377,6 @@ protected MailArchiveChannel getMailArchiveChannel(String mailId) throws RejectE
                         if (log.isDebugEnabled()) {
                             log.debug("Incoming message mailId (" + mailId + ") is NOT a valid site channel reference, will attempt more matches");
                         }
-                    } catch (PermissionException e) {
-                        // INDICATES the channel is valid but the user has no permission to access it
-                        // This generally should not happen because the current user should be the postmaster
-                        log.warn("No access to alias, this should never happen.", e);
-                        // BOUNCE REPLY - send a message back to the user to let them know their email failed
-                        String errMsg = rb.getString("err_not_member") + "\n\n";
-                        String mailSupport = StringUtils.trimToNull(serverConfigurationService.getString("mail.support"));
-                        if (mailSupport != null) {
-                            errMsg += rb.getFormattedMessage("err_questions", mailSupport) + "\n";
-                        }
-                        throw new RejectException(450, errMsg);
                     }
 
                     // next, if not a site, see if it's an alias to a site or channel
@@ -393,20 +409,7 @@ protected MailArchiveChannel getMailArchiveChannel(String mailId) throws RejectE
                         }
 
                         // if there's no channel for this site, it will throw the IdUnusedException caught below
-                        try {
-                            channel = mailArchiveService.getMailArchiveChannel(channelRef);
-                        } catch (PermissionException e) {
-                            // INDICATES the channel is valid but the user has no permission to access it
-                            // This generally should not happen because the current user should be the postmaster
-                            log.warn("No access to alias, this should never happen.", e);
-                            // BOUNCE REPLY - send a message back to the user to let them know their email failed
-                            String errMsg = rb.getString("err_not_member") + "\n\n";
-                            String mailSupport = StringUtils.trimToNull(serverConfigurationService.getString("mail.support"));
-                            if (mailSupport != null) {
-                                errMsg += rb.getFormattedMessage("err_questions", mailSupport) + "\n";
-                            }
-                            throw new RejectException(450, errMsg);
-                        }
+                        channel = mailArchiveService.getMailArchiveChannel(channelRef);
                         if (log.isDebugEnabled()) {
                             log.debug("Incoming message mailId (" + mailId + ") IS a valid channel (" + channelRef + "), found channel: " + channel);
                         }
@@ -466,10 +469,32 @@ protected MailArchiveChannel getMailArchiveChannel(String mailId) throws RejectE
                             errMsg += rb.getFormattedMessage("err_questions", mailSupport) + "\n";
                         }
                         throw new RejectException(450, errMsg);
-                    } else {
-                        return null;
                     }
+                } catch (PermissionException e) {
+                    try {
+                        // Check that we have a postmaster, if we don't then just swallow all mail with warning in logs.
+                        userDirectoryService.getUser(POSTMASTER);
+
+                        // INDICATES the channel is valid but the user has no permission to access it
+                        // This generally should not happen because the current user should be the postmaster
+                        log.warn("No access to alias, this should never happen.", e);
+                        // BOUNCE REPLY - send a message back to the user to let them know their email failed
+                        String errMsg = rb.getString("err_not_member") + "\n\n";
+                        String mailSupport = StringUtils.trimToNull(serverConfigurationService.getString("mail.support"));
+                        if (mailSupport != null) {
+                            errMsg += rb.getFormattedMessage("err_questions", mailSupport) + "\n";
+                        }
+                        throw new RejectException(450, errMsg);
+                    } catch (UserNotDefinedException unde) {
+                        log.warn(String.format("no postmaster, incoming mail will not be processed until a " +
+                                "postmaster user (id=%s) exists in this Sakai instance", POSTMASTER));
+                    }
+                } finally {
+                    session.clear();
                 }
+
+                // Ignore
+                return null;
             }
 
             @Override

mailarchive/mailarchive-subetha/src/test/no-permission.txt

@@ -0,0 +1,8 @@
+From: [email protected]
+To: [email protected]
+Subject: Subject
+Date: Sun, 20 May 2012 13:29:48 +0530
+
+This is a quick test
+
+

mailarchive/mailarchive-subetha/src/test/no-reply.txt

@@ -0,0 +1,8 @@
+From: [email protected]
+To: [email protected]"
+Subject: Subject
+Date: Sun, 20 May 2012 13:29:48 +0530
+
+This is a quick test
+
+

mailarchive/mailarchive-subetha/src/test/org/sakaiproject/mailarchive/SakaiMessageHandlerTest.java

@@ -13,14 +13,18 @@
 import org.sakaiproject.content.api.ContentHostingService;
 import org.sakaiproject.entity.api.EntityManager;
 import org.sakaiproject.exception.IdUnusedException;
+import org.sakaiproject.exception.PermissionException;
 import org.sakaiproject.i18n.InternationalizedMessages;
 import org.sakaiproject.mailarchive.api.MailArchiveChannel;
 import org.sakaiproject.mailarchive.api.MailArchiveService;
 import org.sakaiproject.site.api.SiteService;
 import org.sakaiproject.thread_local.api.ThreadLocalManager;
 import org.sakaiproject.time.api.TimeService;
+import org.sakaiproject.tool.api.Session;
+import org.sakaiproject.tool.api.SessionManager;
 import org.sakaiproject.user.api.User;
 import org.sakaiproject.user.api.UserDirectoryService;
+import org.sakaiproject.user.api.UserNotDefinedException;
 import org.subethamail.smtp.client.SMTPException;
 import org.subethamail.smtp.client.SmartClient;
 import org.subethamail.smtp.server.SMTPServer;
@@ -68,6 +72,12 @@ public class SakaiMessageHandlerTest {
     @Mock
     private MailArchiveService mailArchiveService;
 
+    @Mock
+    private SessionManager sessionManager;
+
+    @Mock
+    private Session session;
+
     private SakaiMessageHandlerFactory messageHandlerFactory;
 
     @Before
@@ -83,12 +93,14 @@ public void setUp() throws Exception {
         messageHandlerFactory.setThreadLocalManager(threadLocalManager);
         messageHandlerFactory.setContentHostingService(contentHostingService);
         messageHandlerFactory.setMailArchiveService(mailArchiveService);
+        messageHandlerFactory.setSessionManager(sessionManager);
 
         // Binding to port 0 means that it picks a random port to listen on.
         when(serverConfigurationService.getInt("smtp.port", 25)).thenReturn(0);
         when(serverConfigurationService.getBoolean("smtp.enabled", false)).thenReturn(true);
         when(serverConfigurationService.getString("sakai.version", "unknown")).thenReturn("1.2.3");
         when(serverConfigurationService.getServerName()).thenReturn("sakai.example.com");
+        when(sessionManager.getCurrentSession()).thenReturn(session);
 
         messageHandlerFactory.init();
     }
@@ -196,8 +208,33 @@ public void testIgnoreNoReply() throws Exception {
         SmartClient client = createClient();
         client.from("[email protected]");
         client.to("[email protected]");
+        writeData(client, "/no-reply.txt");
     }
 
+    @Test(expected = SMTPException.class)
+    public void testNoPermission() throws Exception {
+        // Here there is no permission, this should never happen in real life
+        String reference = "/mailarchive/no-permission/main";
+        when(mailArchiveService.channelReference("no-permission", SiteService.MAIN_CONTAINER)).thenReturn(reference);
+        when(mailArchiveService.getMailArchiveChannel(reference)).thenThrow(PermissionException.class);
+        SmartClient client = createClient();
+        client.from("[email protected]");
+        client.to("[email protected]");
+        writeData(client, "/no-permission.txt");
+    }
+
+    @Test
+    public void testPostmaster() throws Exception {
+        // Here there is no postmaster which may happen and should be dealt with
+        String reference = "/mailarchive/no-permission/main";
+        when(mailArchiveService.channelReference("no-permission", SiteService.MAIN_CONTAINER)).thenReturn(reference);
+        when(mailArchiveService.getMailArchiveChannel(reference)).thenThrow(PermissionException.class);
+        when(userDirectoryService.getUser("postmaster")).thenThrow(UserNotDefinedException.class);
+        SmartClient client = createClient();
+        client.from("[email protected]");
+        client.to("[email protected]");
+        writeData(client, "/no-permission.txt");
+    }
 
     /**
      * Just creates a client connected to the test server.

mailarchive/mailarchive-subetha/src/webapp/WEB-INF/components.xml

@@ -14,6 +14,7 @@
         <property name="aliasService" ref="org.sakaiproject.alias.api.AliasService"/>
         <property name="userDirectoryService" ref="org.sakaiproject.user.api.UserDirectoryService"/>
         <property name="siteService" ref="org.sakaiproject.site.api.SiteService"/>
+        <property name="sessionManager" ref="org.sakaiproject.tool.api.SessionManager"/>
         <property name="timeService" ref="org.sakaiproject.time.api.TimeService"/>
         <property name="threadLocalManager" ref="org.sakaiproject.thread_local.api.ThreadLocalManager"/>
         <property name="contentHostingService" ref="org.sakaiproject.content.api.ContentHostingService"/>