chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1752210677


##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -19,33 +19,58 @@
 
 package org.apache.james.transport.mailets;
 
-import com.github.fge.lambdas.Throwing;
-import com.google.common.collect.ImmutableList;
 import jakarta.inject.Inject;
+import jakarta.inject.Named;
 import jakarta.mail.MessagingException;
-import org.apache.james.core.MailAddress;
+
+import org.apache.james.core.Username;
+import org.apache.james.mailbox.MailboxManager;
+import org.apache.james.mailbox.MailboxSession;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxPath;
 import org.apache.james.user.api.UsersRepository;
 import org.apache.mailet.Mail;
 import org.apache.mailet.StorageDirective;
 import org.apache.mailet.base.GenericMailet;
 
+import com.github.fge.lambdas.Throwing;
+import com.google.common.collect.ImmutableList;
+
 public class SubAddressing extends GenericMailet {
     private final UsersRepository usersRepository;
+    private final MailboxManager mailboxManager;
+    private MailboxSession session;
 
     @Inject
-    public SubAddressing(UsersRepository usersRepository) {
+    public SubAddressing(UsersRepository usersRepository, 
@Named("mailboxmanager") MailboxManager mailboxManager, MailboxSession session) 
{
         this.usersRepository = usersRepository;
+        this.mailboxManager = mailboxManager;
+        this.session = session;
     }
 
     @Override
     public void service(Mail mail) throws MessagingException {
         mail.getRecipients().forEach(recipient ->
             recipient.getLocalPartDetails("+")
                 .ifPresent(
-                    Throwing.consumer(details ->
-                    
StorageDirective.builder().targetFolders(ImmutableList.of(details)).build()
-                        
.encodeAsAttributes(usersRepository.getUsername(recipient))
-                        .forEach(mail::setAttribute))
-                ));
+                    Throwing.consumer(details -> {
+
+                        boolean hasRight = mailboxManager.hasRight(
+                            MailboxPath.forUser(
+                                
Username.fromMailAddress(recipient.stripDetails("+")),
+                                details
+                            ),
+                            MailboxACL.Right.Post,
+                            session
+                        );
+
+                        String targetFolder = hasRight ? details : "inbox";

Review Comment:
   Two things
   
    -> I am at war with `?` operator. It do not read like english and actually 
make reading the code more complex. Generally `if` reads better.
    
    -> `"inbox"` shall be a constant and not a crafted value. 
MailboxContants.INBOX is better.
    
    And finally if the guy has no right, do we need to do anything at all? 
After all, if we don't do anything, then it would land onto the recipient 
mailbox and if some prior storage directives had been positionned (spam 
checking?) we would not override it! 
    
    (that said maybe an INFO log saying "oups bob tried to address alice 
subfolder but did not had the right to do it" would be great!)
    
    Sorry finally it is 4 things...



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to