You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by "florentos17 (via GitHub)" <gi...@apache.org> on 2024/09/10 14:55:16 UTC
[PR] James 3945 [james-project]
florentos17 opened a new pull request, #2405:
URL: https://github.com/apache/james-project/pull/2405
i am working on implementing sub-addressing for apache james.
for now it works but WITHOUT any right management.
this is still work in progress.
any comment is welcome :)
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1752199863
##########
examples/custom-james-assembly/custom-james-assembly.iml:
##########
@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="UTF-8"?>
Review Comment:
Please configure a local gitignore onto your laptop so that you do not commit ide file into an Apache project
(without modifying the project gitignore)
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on PR #2405:
URL: https://github.com/apache/james-project/pull/2405#issuecomment-2356995535
Can we stop force pushing all the time? I'd rather review minimal fixups...
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on PR #2405:
URL: https://github.com/apache/james-project/pull/2405#issuecomment-2355339904
- We miss documentation of this new mailet
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1764629590
##########
server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/SubAddressingTest.java:
##########
@@ -96,40 +97,79 @@ void shouldNotAddStorageDirectiveWhenNoRight() throws Exception {
@Test
void shouldAddStorageDirectiveMatchingDetailsWhenRight() throws Exception {
- MailboxSession session = mailboxManager.createSystemSession(Username.of("recipient"));
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
// create Mailbox
MailboxId mailboxId = mailboxManager.createMailbox(
- MailboxPath.forUser(Username.of("recipient"), "any"), session)
- .get();
+ MailboxPath.forUser(username, "any"), session).get();
- // give right to post
- MailboxACL.ACLCommand command = MailboxACL.command()
- .key(MailboxACL.ANYBODY_KEY)
- .rights(MailboxACL.Right.Post)
- .asAddition();
+ giveRightToPost(mailboxId, session);
- mailboxManager.applyRightsCommand(mailboxId, command, session);
+ Mail mail = sendSubAddressedMail(true);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, "any"));
+ }
+
+ @Test
+ void shouldAddStorageDirectiveWhenEveryoneHasRightAndSenderIsUnknown() throws Exception {
+
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
+
+ // create Mailbox
+ MailboxId mailboxId = mailboxManager.createMailbox(
+ MailboxPath.forUser(username, "any"), session).get();
+
+ giveRightToPost(mailboxId, session);
+ Mail mail = sendSubAddressedMail(false);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, "any"));
+ }
+
+ @Test
+ void shouldNotAddStorageDirectiveWhenNooneHasRightAndSenderIsUnknown() throws Exception {
+
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
+
+ // create Mailbox
+ MailboxId mailboxId = mailboxManager.createMailbox(
+ MailboxPath.forUser(username, "any"), session).get();
- Mail mail = sendSubAddressedMail();
+ giveRightToPost(mailboxId, session);
+ Mail mail = sendSubAddressedMail(false);
AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
assertThat(mail.attributes().map(this::unbox))
.doesNotContain(Pair.of(recipient, "any"));
}
- private Mail sendSubAddressedMail() throws MessagingException, IOException {
+ private void giveRightToPost(MailboxId mailboxId, MailboxSession session) throws MailboxException {
Review Comment:
Yes IMO ok
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1758756361
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -19,33 +19,86 @@
package org.apache.james.transport.mailets;
-import com.github.fge.lambdas.Throwing;
-import com.google.common.collect.ImmutableList;
+import static org.apache.james.mailbox.MessageManager.MailboxMetaData.RecentMode.IGNORE;
+
+import java.util.Optional;
+
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.MessageManager;
+import org.apache.james.mailbox.exception.MailboxException;
+import org.apache.james.mailbox.exception.MailboxNotFoundException;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxPath;
import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.api.UsersRepositoryException;
import org.apache.mailet.Mail;
import org.apache.mailet.StorageDirective;
import org.apache.mailet.base.GenericMailet;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+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 static final Logger LOG = LoggerFactory.getLogger(SubAddressing.class);
@Inject
- public SubAddressing(UsersRepository usersRepository) {
+ public SubAddressing(UsersRepository usersRepository, @Named("mailboxmanager") MailboxManager mailboxManager) {
this.usersRepository = usersRepository;
+ this.mailboxManager = mailboxManager;
}
@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))
- ));
+ recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
+ .ifPresent(Throwing.consumer(targetFolder -> postIfHasRight(mail, recipient, targetFolder))));
+ }
+
+ private void postIfHasRight(Mail mail, MailAddress recipient, String targetFolder) throws UsersRepositoryException, MailboxException {
+ if (hasPostRight(recipient, targetFolder)) {
+ StorageDirective.builder().targetFolders(ImmutableList.of(targetFolder)).build()
+ .encodeAsAttributes(usersRepository.getUsername(recipient))
+ .forEach(mail::setAttribute);
+ } else {
+ LOG.info("{} tried to address {}'s subfolder `{}` but did not have the right to",
+ mail.getMaybeSender().toString(), recipient, targetFolder);
+ }
+ }
+
+ private Boolean hasPostRight(MailAddress recipient, String targetFolder) throws MailboxException {
+
+ try {
Review Comment:
remove this extra blank line
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1764061474
##########
server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/SubAddressingTest.java:
##########
@@ -96,40 +97,79 @@ void shouldNotAddStorageDirectiveWhenNoRight() throws Exception {
@Test
void shouldAddStorageDirectiveMatchingDetailsWhenRight() throws Exception {
- MailboxSession session = mailboxManager.createSystemSession(Username.of("recipient"));
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
// create Mailbox
MailboxId mailboxId = mailboxManager.createMailbox(
- MailboxPath.forUser(Username.of("recipient"), "any"), session)
- .get();
+ MailboxPath.forUser(username, "any"), session).get();
- // give right to post
- MailboxACL.ACLCommand command = MailboxACL.command()
- .key(MailboxACL.ANYBODY_KEY)
- .rights(MailboxACL.Right.Post)
- .asAddition();
+ giveRightToPost(mailboxId, session);
- mailboxManager.applyRightsCommand(mailboxId, command, session);
+ Mail mail = sendSubAddressedMail(true);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, "any"));
+ }
+
+ @Test
+ void shouldAddStorageDirectiveWhenEveryoneHasRightAndSenderIsUnknown() throws Exception {
+
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
+
+ // create Mailbox
+ MailboxId mailboxId = mailboxManager.createMailbox(
+ MailboxPath.forUser(username, "any"), session).get();
+
+ giveRightToPost(mailboxId, session);
+ Mail mail = sendSubAddressedMail(false);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, "any"));
+ }
+
+ @Test
+ void shouldNotAddStorageDirectiveWhenNooneHasRightAndSenderIsUnknown() throws Exception {
+
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
+
+ // create Mailbox
+ MailboxId mailboxId = mailboxManager.createMailbox(
+ MailboxPath.forUser(username, "any"), session).get();
- Mail mail = sendSubAddressedMail();
+ giveRightToPost(mailboxId, session);
+ Mail mail = sendSubAddressedMail(false);
AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
assertThat(mail.attributes().map(this::unbox))
.doesNotContain(Pair.of(recipient, "any"));
}
- private Mail sendSubAddressedMail() throws MessagingException, IOException {
+ private void giveRightToPost(MailboxId mailboxId, MailboxSession session) throws MailboxException {
+ MailboxACL.ACLCommand command = MailboxACL.command()
+ .key(MailboxACL.ANYBODY_KEY)
+ .rights(MailboxACL.Right.Post)
+ .asAddition();
+
+ mailboxManager.applyRightsCommand(mailboxId, command, session);
+ }
+
+ private Mail sendSubAddressedMail(boolean knownSender) throws MessagingException, IOException {
Review Comment:
init of the mailet has nothing to do with sending mail. Can we move it in @BeforeEach ?
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1757142525
##########
examples/custom-james-assembly/src/main/java/org/apache/james/examples/CustomJamesServerMain.java:
##########
@@ -60,22 +61,29 @@ public class CustomJamesServerMain implements JamesServerMain {
new MemoryMailQueueModule(),
new TaskManagerModule(),
new RawPostDequeueDecoratorModule(),
- binder -> binder.bind(MailetContainerModule.DefaultProcessorsConfigurationSupplier.class)
- .toInstance(BaseHierarchicalConfiguration::new));
+ binder -> binder.bind(
+ MailetContainerModule
+ .DefaultProcessorsConfigurationSupplier.class
+ ).toInstance(BaseHierarchicalConfiguration::new));
Review Comment:
```suggestion
binder -> binder.bind(MailetContainerModule .DefaultProcessorsConfigurationSupplier.class)
.toInstance(BaseHierarchicalConfiguration::new));
```
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1763026972
##########
server/mailet/integration-testing/src/test/java/org/apache/james/mailets/SubAddressingTest.java:
##########
@@ -0,0 +1,155 @@
+/****************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one *
+ * or more contributor license agreements. See the NOTICE file *
+ * distributed with this work for additional information *
+ * regarding copyright ownership. The ASF licenses this file *
+ * to you under the Apache License, Version 2.0 (the *
+ * "License"); you may not use this file except in compliance *
+ * with the License. You may obtain a copy of the License at *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
+ * Unless required by applicable law or agreed to in writing, *
+ * software distributed under the License is distributed on an *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
+ * KIND, either express or implied. See the License for the *
+ * specific language governing permissions and limitations *
+ * under the License. *
+ ****************************************************************/
+
+package org.apache.james.mailets;
+
+
+import static org.apache.james.mailets.SPFIntegrationTests.POSTMASTER;
+import static org.apache.james.mailets.configuration.Constants.DEFAULT_DOMAIN;
+import static org.apache.james.mailets.configuration.Constants.FROM;
+import static org.apache.james.mailets.configuration.Constants.LOCALHOST_IP;
+import static org.apache.james.mailets.configuration.Constants.PASSWORD;
+import static org.apache.james.mailets.configuration.Constants.RECIPIENT;
+import static org.apache.james.mailets.configuration.Constants.awaitAtMostOneMinute;
+
+import java.io.File;
+import java.io.IOException;
+import java.security.InvalidKeyException;
+import java.security.NoSuchAlgorithmException;
+import java.security.spec.InvalidKeySpecException;
+
+import org.apache.james.core.Username;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxConstants;
+import org.apache.james.mailbox.model.MailboxPath;
+import org.apache.james.mailets.configuration.CommonProcessors;
+import org.apache.james.mailets.configuration.MailetConfiguration;
+import org.apache.james.mailets.configuration.ProcessorConfiguration;
+import org.apache.james.modules.ACLProbeImpl;
+import org.apache.james.modules.protocols.ImapGuiceProbe;
+import org.apache.james.modules.protocols.SmtpGuiceProbe;
+import org.apache.james.probe.DataProbe;
+import org.apache.james.transport.mailets.SubAddressing;
+import org.apache.james.transport.matchers.All;
+import org.apache.james.utils.DataProbeImpl;
+import org.apache.james.utils.SMTPMessageSender;
+import org.apache.james.utils.TestIMAPClient;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.api.io.TempDir;
+
+
+class SubAddressingTest {
+ private static final String TARGETED_MAILBOX = "any";
+
+ @RegisterExtension
+ public TestIMAPClient testIMAPClient = new TestIMAPClient();
+ @RegisterExtension
+ public SMTPMessageSender messageSender = new SMTPMessageSender(DEFAULT_DOMAIN);
+
+ private TemporaryJamesServer jamesServer;
+
+ void setup(File temporaryFolder) throws Exception {
+ jamesServer = TemporaryJamesServer.builder()
+ .withMailetContainer(TemporaryJamesServer.defaultMailetContainerConfiguration()
+ .postmaster(POSTMASTER)
+ .putProcessor(ProcessorConfiguration.transport()
+ .addMailet(MailetConfiguration.builder()
+ .matcher(All.class)
+ .mailet(SubAddressing.class))
+ .addMailetsFrom(CommonProcessors.transport())))
+ .build(temporaryFolder);
+ jamesServer.start();
+
+ DataProbe dataProbe = jamesServer.getProbe(DataProbeImpl.class);
+ dataProbe.addDomain(DEFAULT_DOMAIN);
+ dataProbe.addUser(RECIPIENT, PASSWORD);
+ dataProbe.addUser(FROM, PASSWORD);
+ }
+
+ @AfterEach
+ void tearDown() throws IOException {
+ if (jamesServer != null) {
+ jamesServer.shutdown();
+ }
+ testIMAPClient.close();
+ messageSender.close();
+ }
+
+ @Test
+ void subAddressedEmailShouldBeDeliveredInINBOXWhenSpecifiedFolderDoesNotExist(@TempDir File temporaryFolder) throws Exception {
+ setup(temporaryFolder);
+
+ //do not create mailbox
+
+ sendMail();
Review Comment:
```suggestion
sendMailToSubAddress();
```
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1766134374
##########
server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/SubAddressingTest.java:
##########
@@ -0,0 +1,220 @@
+/****************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one *
+ * or more contributor license agreements. See the NOTICE file *
+ * distributed with this work for additional information *
+ * regarding copyright ownership. The ASF licenses this file *
+ * to you under the Apache License, Version 2.0 (the *
+ * "License"); you may not use this file except in compliance *
+ * with the License. You may obtain a copy of the License at *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
+ * Unless required by applicable law or agreed to in writing, *
+ * software distributed under the License is distributed on an *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
+ * KIND, either express or implied. See the License for the *
+ * specific language governing permissions and limitations *
+ * under the License. *
+ ****************************************************************/
+
+package org.apache.james.transport.mailets;
+
+import static org.apache.james.transport.mailets.WithStorageDirectiveTest.NO_DOMAIN_LIST;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Collection;
+
+import jakarta.mail.MessagingException;
+
+import org.apache.commons.lang3.tuple.Pair;
+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.exception.MailboxException;
+import org.apache.james.mailbox.inmemory.manager.InMemoryIntegrationResources;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxId;
+import org.apache.james.mailbox.model.MailboxPath;
+import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.api.UsersRepositoryException;
+import org.apache.james.user.memory.MemoryUsersRepository;
+import org.apache.mailet.Attribute;
+import org.apache.mailet.AttributeName;
+import org.apache.mailet.AttributeValue;
+import org.apache.mailet.Mail;
+import org.apache.mailet.base.test.FakeMail;
+import org.apache.mailet.base.test.FakeMailetConfig;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Disabled;
+import org.junit.jupiter.api.Test;
+
+
+public class SubAddressingTest {
+
+ private static final String SENDER1 = "sender1@localhost";
+ private static final String SENDER2 = "sender2@localhost";
+ private static final String RECIPIENT = "recipient@localhost";
+ private static final String TARGET = "targetfolder";
+ private static final String UNEXISTING_TARGET = "unexistingfolder";
+
+ private MailboxManager mailboxManager;
+ private SubAddressing testee;
+ private UsersRepository usersRepository;
+ private Username sender1Username;
+ private Username recipientUsername;
+ private MailboxSession recipientSession;
+ private MailboxId targetMailboxId;
+
+ @BeforeEach
+ void setUp() throws MailboxException, MessagingException, UsersRepositoryException {
+ mailboxManager = InMemoryIntegrationResources.defaultResources().getMailboxManager();
+ usersRepository = MemoryUsersRepository.withVirtualHosting(NO_DOMAIN_LIST);
+
+ recipientUsername = usersRepository.getUsername(new MailAddress(RECIPIENT));
+ sender1Username = usersRepository.getUsername(new MailAddress(SENDER1));
+
+ recipientSession = mailboxManager.createSystemSession(recipientUsername);
+ targetMailboxId = mailboxManager.createMailbox(
+ MailboxPath.forUser(recipientUsername, TARGET), recipientSession).get();
+
+ testee = new SubAddressing(usersRepository, mailboxManager);
+ testee.init(FakeMailetConfig.builder().build());
+ }
+
+ @Test
+ void shouldNotAddStorageDirectiveWhenTargetMailboxDoesNotExist() throws Exception {
+ Mail mail = mailBuilder(UNEXISTING_TARGET).sender(SENDER1).build();
+ testee.service(mail);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, UNEXISTING_TARGET));
+ }
+
+ @Test
+ void shouldNotAddStorageDirectiveWhenNobodyHasRight() throws Exception {
+ givePostRightForKey(MailboxACL.ANYBODY_NEGATIVE_KEY);
+
+ Mail mail = mailBuilder(TARGET).sender(SENDER1).build();
+ testee.service(mail);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, TARGET));
+ }
+
+
+ @Test
+ void shouldAddStorageDirectiveWhenAnybodyHasRight() throws Exception {
+ givePostRightForKey(MailboxACL.ANYBODY_KEY);
+
+ Mail mail = mailBuilder(TARGET).sender(SENDER1).build();
+ testee.service(mail);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .containsOnly(Pair.of(recipient, TARGET));
+ }
+
+ @Disabled
Review Comment:
Why is this test disabled? Usually we leave a reason with it like, so that if one day we want to fix it, we can remember quickly what was wrong in the first place
```
@Disabled("[]REASON]")
```
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "florentos17 (via GitHub)" <gi...@apache.org>.
florentos17 commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1766459634
##########
server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/SubAddressingTest.java:
##########
@@ -0,0 +1,220 @@
+/****************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one *
+ * or more contributor license agreements. See the NOTICE file *
+ * distributed with this work for additional information *
+ * regarding copyright ownership. The ASF licenses this file *
+ * to you under the Apache License, Version 2.0 (the *
+ * "License"); you may not use this file except in compliance *
+ * with the License. You may obtain a copy of the License at *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
+ * Unless required by applicable law or agreed to in writing, *
+ * software distributed under the License is distributed on an *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
+ * KIND, either express or implied. See the License for the *
+ * specific language governing permissions and limitations *
+ * under the License. *
+ ****************************************************************/
+
+package org.apache.james.transport.mailets;
+
+import static org.apache.james.transport.mailets.WithStorageDirectiveTest.NO_DOMAIN_LIST;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Collection;
+
+import jakarta.mail.MessagingException;
+
+import org.apache.commons.lang3.tuple.Pair;
+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.exception.MailboxException;
+import org.apache.james.mailbox.inmemory.manager.InMemoryIntegrationResources;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxId;
+import org.apache.james.mailbox.model.MailboxPath;
+import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.api.UsersRepositoryException;
+import org.apache.james.user.memory.MemoryUsersRepository;
+import org.apache.mailet.Attribute;
+import org.apache.mailet.AttributeName;
+import org.apache.mailet.AttributeValue;
+import org.apache.mailet.Mail;
+import org.apache.mailet.base.test.FakeMail;
+import org.apache.mailet.base.test.FakeMailetConfig;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Disabled;
+import org.junit.jupiter.api.Test;
+
+
+public class SubAddressingTest {
+
+ private static final String SENDER1 = "sender1@localhost";
+ private static final String SENDER2 = "sender2@localhost";
+ private static final String RECIPIENT = "recipient@localhost";
+ private static final String TARGET = "targetfolder";
+ private static final String UNEXISTING_TARGET = "unexistingfolder";
+
+ private MailboxManager mailboxManager;
+ private SubAddressing testee;
+ private UsersRepository usersRepository;
+ private Username sender1Username;
+ private Username recipientUsername;
+ private MailboxSession recipientSession;
+ private MailboxId targetMailboxId;
+
+ @BeforeEach
+ void setUp() throws MailboxException, MessagingException, UsersRepositoryException {
+ mailboxManager = InMemoryIntegrationResources.defaultResources().getMailboxManager();
+ usersRepository = MemoryUsersRepository.withVirtualHosting(NO_DOMAIN_LIST);
+
+ recipientUsername = usersRepository.getUsername(new MailAddress(RECIPIENT));
+ sender1Username = usersRepository.getUsername(new MailAddress(SENDER1));
+
+ recipientSession = mailboxManager.createSystemSession(recipientUsername);
+ targetMailboxId = mailboxManager.createMailbox(
+ MailboxPath.forUser(recipientUsername, TARGET), recipientSession).get();
+
+ testee = new SubAddressing(usersRepository, mailboxManager);
+ testee.init(FakeMailetConfig.builder().build());
+ }
+
+ @Test
+ void shouldNotAddStorageDirectiveWhenTargetMailboxDoesNotExist() throws Exception {
+ Mail mail = mailBuilder(UNEXISTING_TARGET).sender(SENDER1).build();
+ testee.service(mail);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, UNEXISTING_TARGET));
+ }
+
+ @Test
+ void shouldNotAddStorageDirectiveWhenNobodyHasRight() throws Exception {
+ givePostRightForKey(MailboxACL.ANYBODY_NEGATIVE_KEY);
+
+ Mail mail = mailBuilder(TARGET).sender(SENDER1).build();
+ testee.service(mail);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, TARGET));
+ }
+
+
+ @Test
+ void shouldAddStorageDirectiveWhenAnybodyHasRight() throws Exception {
+ givePostRightForKey(MailboxACL.ANYBODY_KEY);
+
+ Mail mail = mailBuilder(TARGET).sender(SENDER1).build();
+ testee.service(mail);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .containsOnly(Pair.of(recipient, TARGET));
+ }
+
+ @Disabled
Review Comment:
final fixups were required as the test was still red, but it should be good now ;)
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1765051923
##########
server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/SubAddressingTest.java:
##########
@@ -0,0 +1,213 @@
+/****************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one *
+ * or more contributor license agreements. See the NOTICE file *
+ * distributed with this work for additional information *
+ * regarding copyright ownership. The ASF licenses this file *
+ * to you under the Apache License, Version 2.0 (the *
+ * "License"); you may not use this file except in compliance *
+ * with the License. You may obtain a copy of the License at *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
+ * Unless required by applicable law or agreed to in writing, *
+ * software distributed under the License is distributed on an *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
+ * KIND, either express or implied. See the License for the *
+ * specific language governing permissions and limitations *
+ * under the License. *
+ ****************************************************************/
+
+package org.apache.james.transport.mailets;
+
+import static org.apache.james.transport.mailets.WithStorageDirectiveTest.NO_DOMAIN_LIST;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Collection;
+
+import jakarta.mail.MessagingException;
+
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.james.core.Username;
+import org.apache.james.mailbox.MailboxManager;
+import org.apache.james.mailbox.MailboxSession;
+import org.apache.james.mailbox.exception.MailboxException;
+import org.apache.james.mailbox.inmemory.manager.InMemoryIntegrationResources;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxId;
+import org.apache.james.mailbox.model.MailboxPath;
+import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.memory.MemoryUsersRepository;
+import org.apache.mailet.Attribute;
+import org.apache.mailet.AttributeName;
+import org.apache.mailet.AttributeValue;
+import org.apache.mailet.Mail;
+import org.apache.mailet.base.test.FakeMail;
+import org.apache.mailet.base.test.FakeMailetConfig;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+
+public class SubAddressingTest {
+
+ private static final String SENDER1 = "sender1@localhost";
+ private static final String SENDER2 = "sender2@localhost";
+ private static final String TARGET = "targetfolder";
+ private static final String UNEXISTING_TARGET = "unexistingfolder";
+
+ private MailboxManager mailboxManager;
+ private SubAddressing testee;
+ private UsersRepository usersRepository;
+ private Username sender1Username;
+ private Username sender2Username;
+ private Username recipientUsername;
+ private MailboxSession recipientSession;
+ private MailboxId targetMailboxId;
+
+ @BeforeEach
+ void setUp() throws MailboxException, MessagingException {
+ mailboxManager = InMemoryIntegrationResources.defaultResources().getMailboxManager();
+ usersRepository = MemoryUsersRepository.withVirtualHosting(NO_DOMAIN_LIST);
+
+ testee = new SubAddressing(usersRepository, mailboxManager);
+ testee.init(FakeMailetConfig.builder()
+ .build());
+
+ sender1Username = Username.of("sender1");
+ sender2Username = Username.of("sender2");
+ recipientUsername = Username.of("recipient");
+ recipientSession = mailboxManager.createSystemSession(recipientUsername);
+ targetMailboxId = mailboxManager.createMailbox(
+ MailboxPath.forUser(recipientUsername, TARGET), recipientSession).get();
+ }
+
+ @Test
+ void shouldNotAddStorageDirectiveWhenTargetMailboxDoesNotExist() throws Exception {
+ Mail mail = mailBuilder(UNEXISTING_TARGET).sender(SENDER1).build();
+ testee.service(mail);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, UNEXISTING_TARGET));
+ }
+
+ @Test
+ void shouldNotAddStorageDirectiveWhenNobodyHasRight() throws Exception {
+ givePostRightForKey(MailboxACL.ANYBODY_NEGATIVE_KEY);
+
+ Mail mail = mailBuilder(TARGET).sender(SENDER1).build();
+ testee.service(mail);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, TARGET));
+ }
+
+
+ @Test
+ void shouldAddStorageDirectiveWhenAnybodyHasRight() throws Exception {
+ givePostRightForKey(MailboxACL.ANYBODY_KEY);
+
+ Mail mail = mailBuilder(TARGET).sender(SENDER1).build();
+ testee.service(mail);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .containsOnly(Pair.of(recipient, TARGET));
+ }
+
+ @Test
+ void shouldAddStorageDirectiveWhenSenderIsWhiteListed() throws Exception {
+ // whitelist sender 1 and send from sender 1
+ givePostRightForKey(MailboxACL.EntryKey.createUserEntryKey(sender1Username, false));
+
+ Mail mail = mailBuilder(TARGET).sender(SENDER1).build();
+ testee.service(mail);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .containsOnly(Pair.of(recipient, TARGET));
+ }
+
+ @Test
+ void shouldNotAddStorageDirectiveWhenSenderIsNotWhiteListed() throws Exception {
+ // whitelist sender 1 and send from sender 2
+ givePostRightForKey(MailboxACL.EntryKey.createUserEntryKey(sender1Username, false));
+
+ Mail mail = mailBuilder(TARGET).sender(SENDER2).build();
+ testee.service(mail);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, TARGET));
+ }
+
+ @Test
+ void shouldNotAddStorageDirectiveWhenSenderIsBlackListed() throws Exception {
+ // blacklist sender 1 and send from sender 1
+ givePostRightForKey(MailboxACL.EntryKey.createUserEntryKey(sender1Username, true));
Review Comment:
Grant rights to everybody before restricting them for sender1 otherwise you do not assert anything!
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "florentos17 (via GitHub)" <gi...@apache.org>.
florentos17 commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1754976317
##########
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:
1. `?` operator & 2. MailboxContants.INBOX -> done
3. don't do anything if no right & 4. INFO log are still todo
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1764066377
##########
server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/SubAddressingTest.java:
##########
@@ -96,40 +97,79 @@ void shouldNotAddStorageDirectiveWhenNoRight() throws Exception {
@Test
void shouldAddStorageDirectiveMatchingDetailsWhenRight() throws Exception {
- MailboxSession session = mailboxManager.createSystemSession(Username.of("recipient"));
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
// create Mailbox
MailboxId mailboxId = mailboxManager.createMailbox(
- MailboxPath.forUser(Username.of("recipient"), "any"), session)
- .get();
+ MailboxPath.forUser(username, "any"), session).get();
- // give right to post
- MailboxACL.ACLCommand command = MailboxACL.command()
- .key(MailboxACL.ANYBODY_KEY)
- .rights(MailboxACL.Right.Post)
- .asAddition();
+ giveRightToPost(mailboxId, session);
- mailboxManager.applyRightsCommand(mailboxId, command, session);
+ Mail mail = sendSubAddressedMail(true);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, "any"));
+ }
+
+ @Test
+ void shouldAddStorageDirectiveWhenEveryoneHasRightAndSenderIsUnknown() throws Exception {
+
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
Review Comment:
Relocate this boiler plate in @beforeEach, creating related class field as needed
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1752184972
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -0,0 +1,51 @@
+/****************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one *
+ * or more contributor license agreements. See the NOTICE file *
+ * distributed with this work for additional information *
+ * regarding copyright ownership. The ASF licenses this file *
+ * to you under the Apache License, Version 2.0 (the *
+ * "License"); you may not use this file except in compliance *
+ * with the License. You may obtain a copy of the License at *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
+ * Unless required by applicable law or agreed to in writing, *
+ * software distributed under the License is distributed on an *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
+ * KIND, either express or implied. See the License for the *
+ * specific language governing permissions and limitations *
+ * under the License. *
+ ****************************************************************/
+
+package org.apache.james.transport.mailets;
+
+import com.github.fge.lambdas.Throwing;
+import com.google.common.collect.ImmutableList;
+import jakarta.inject.Inject;
+import jakarta.mail.MessagingException;
+import org.apache.james.core.MailAddress;
+import org.apache.james.user.api.UsersRepository;
+import org.apache.mailet.Mail;
+import org.apache.mailet.StorageDirective;
+import org.apache.mailet.base.GenericMailet;
+
+public class SubAddressing extends GenericMailet {
+ private final UsersRepository usersRepository;
+
+ @Inject
+ public SubAddressing(UsersRepository usersRepository) {
+ this.usersRepository = usersRepository;
+ }
+
+ @Override
+ public void service(Mail mail) throws MessagingException {
+ mail.getRecipients().forEach(recipient ->
+ recipient.getLocalPartDetails("+")
Review Comment:
Ideally we should define the `LOCALPART_DETAIL_DELIMITER` constant into the UsersRepository interface for reuse.
As of today it is duplicated and thus it creates coupling.
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on PR #2405:
URL: https://github.com/apache/james-project/pull/2405#issuecomment-2341212515
To ease linking individual commits to a JIRA issue we generally put the JIRA issue as a prefix of the commit messages.
Can you reword your commit messages so that they look like `JAMES-3945 What I have to say...` ?
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1754965894
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -55,20 +63,36 @@ public void service(Mail mail) throws MessagingException {
.ifPresent(
Throwing.consumer(details -> {
- boolean hasRight = mailboxManager.hasRight(
- MailboxPath.forUser(
- Username.fromMailAddress(recipient.stripDetails("+")),
- details
- ),
- MailboxACL.Right.Post,
- session
- );
+ boolean hasRight;
+
+ try {
+ Username recipientUsername = Username.fromLocalPartWithoutDomain(recipient.getLocalPartWithoutDetails("+"));
+ MailboxSession session = mailboxManager.createSystemSession(recipientUsername);
- String targetFolder = hasRight ? details : "inbox";
+ MailboxACL aclCommand = mailboxManager.getMailbox(
+ MailboxPath.forUser(recipientUsername, details), session)
+ .getMetaData(IGNORE, session, MessageManager.MailboxMetaData.FetchGroup.NO_COUNT)
+ .getACL();
+ hasRight = aclCommand.getEntries().containsKey(MailboxACL.ANYBODY_KEY) &&
+ aclCommand.getEntries().get(MailboxACL.ANYBODY_KEY).equals(MailboxACL.Rfc4314Rights.of(Collections.singletonList(MailboxACL.Right.Post)));
+
+ } catch (MailboxNotFoundException e) {
+ hasRight = false;
+ }
+
+ String targetFolder;
+ if(hasRight) {
+ targetFolder = details;
+ } else {
+ // TODO find LOG
+ // LOG.info(maybesender + " tried to address " + recipient + "'s subfolder `" + details + "` but did not have the right to")
+ targetFolder = MailboxConstants.INBOX;
Review Comment:
Nope else we just do not do something. IE we only put the storage directive when we have rights?
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1754958468
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -55,20 +63,36 @@ public void service(Mail mail) throws MessagingException {
.ifPresent(
Throwing.consumer(details -> {
- boolean hasRight = mailboxManager.hasRight(
- MailboxPath.forUser(
- Username.fromMailAddress(recipient.stripDetails("+")),
- details
- ),
- MailboxACL.Right.Post,
- session
- );
+ boolean hasRight;
+
+ try {
+ Username recipientUsername = Username.fromLocalPartWithoutDomain(recipient.getLocalPartWithoutDetails("+"));
+ MailboxSession session = mailboxManager.createSystemSession(recipientUsername);
- String targetFolder = hasRight ? details : "inbox";
+ MailboxACL aclCommand = mailboxManager.getMailbox(
+ MailboxPath.forUser(recipientUsername, details), session)
+ .getMetaData(IGNORE, session, MessageManager.MailboxMetaData.FetchGroup.NO_COUNT)
+ .getACL();
+ hasRight = aclCommand.getEntries().containsKey(MailboxACL.ANYBODY_KEY) &&
Review Comment:
Do not re-allocate variables
Generally method extraction is a good alternative.
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1754968725
##########
server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/SubAddressingTest.java:
##########
@@ -59,37 +64,48 @@ public class SubAddressingTest {
@BeforeEach
void setUp() throws MailboxException {
- usersRepository = mock(UsersRepository.class);
+ usersRepository = MemoryUsersRepository.withVirtualHosting(NO_DOMAIN_LIST);
mailboxManager = InMemoryIntegrationResources.defaultResources().getMailboxManager();
- MailboxSession session = mailboxManager.createSystemSession(Username.of("recipient"));
+ MetricFactory metricFactory = new RecordingMetricFactory();
+
+ testee = new SubAddressing(usersRepository, mailboxManager, metricFactory);
+ }
- mailboxManager.createMailbox(MailboxPath.forUser(Username.of("recipient"), "any"), session);
- testee = new SubAddressing(usersRepository, mailboxManager, session);
+ @Test
+ void shouldNotAddStorageDirectiveWhenNoRight() throws Exception {
Review Comment:
We need integration tests for this too
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1757144396
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -0,0 +1,106 @@
+/****************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one *
+ * or more contributor license agreements. See the NOTICE file *
+ * distributed with this work for additional information *
+ * regarding copyright ownership. The ASF licenses this file *
+ * to you under the Apache License, Version 2.0 (the *
+ * "License"); you may not use this file except in compliance *
+ * with the License. You may obtain a copy of the License at *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
+ * Unless required by applicable law or agreed to in writing, *
+ * software distributed under the License is distributed on an *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
+ * KIND, either express or implied. See the License for the *
+ * specific language governing permissions and limitations *
+ * under the License. *
+ ****************************************************************/
+
+package org.apache.james.transport.mailets;
+
+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.MessageManager;
+import org.apache.james.mailbox.exception.MailboxException;
+import org.apache.james.mailbox.exception.MailboxNotFoundException;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxConstants;
+import org.apache.james.mailbox.model.MailboxPath;
+import org.apache.james.metrics.api.MetricFactory;
+import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.api.UsersRepositoryException;
+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;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collections;
+
+import static org.apache.james.mailbox.MessageManager.MailboxMetaData.RecentMode.IGNORE;
+
+public class SubAddressing extends GenericMailet {
+ private final UsersRepository usersRepository;
+ private final MailboxManager mailboxManager;
+ private final MetricFactory metricFactory;
+
+ private static final Logger LOG = LoggerFactory.getLogger(SubAddressing.class);
+
+ @Inject
+ public SubAddressing(UsersRepository usersRepository, @Named("mailboxmanager") MailboxManager mailboxManager, MetricFactory metricFactory) {
+ this.usersRepository = usersRepository;
+ this.mailboxManager = mailboxManager;
+ this.metricFactory = metricFactory;
+ }
+
+ @Override
+ public void service(Mail mail) throws MessagingException {
+ mail.getRecipients().forEach(recipient ->
+ recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
+ .ifPresent(
+ Throwing.consumer(targetFolder -> postIfHasRight(mail, recipient, targetFolder))
+ )
+ );
Review Comment:
```suggestion
mail.getRecipients().forEach(recipient ->
recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
.ifPresent(Throwing.consumer(targetFolder -> postIfHasRight(mail, recipient, targetFolder))));
```
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "Arsnael (via GitHub)" <gi...@apache.org>.
Arsnael commented on PR #2405:
URL: https://github.com/apache/james-project/pull/2405#issuecomment-2362807344
Good to merge correct?
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1764063508
##########
server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/SubAddressingTest.java:
##########
@@ -96,40 +97,79 @@ void shouldNotAddStorageDirectiveWhenNoRight() throws Exception {
@Test
void shouldAddStorageDirectiveMatchingDetailsWhenRight() throws Exception {
- MailboxSession session = mailboxManager.createSystemSession(Username.of("recipient"));
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
// create Mailbox
MailboxId mailboxId = mailboxManager.createMailbox(
- MailboxPath.forUser(Username.of("recipient"), "any"), session)
- .get();
+ MailboxPath.forUser(username, "any"), session).get();
- // give right to post
- MailboxACL.ACLCommand command = MailboxACL.command()
- .key(MailboxACL.ANYBODY_KEY)
- .rights(MailboxACL.Right.Post)
- .asAddition();
+ giveRightToPost(mailboxId, session);
- mailboxManager.applyRightsCommand(mailboxId, command, session);
+ Mail mail = sendSubAddressedMail(true);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, "any"));
+ }
+
+ @Test
+ void shouldAddStorageDirectiveWhenEveryoneHasRightAndSenderIsUnknown() throws Exception {
+
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
+
+ // create Mailbox
+ MailboxId mailboxId = mailboxManager.createMailbox(
+ MailboxPath.forUser(username, "any"), session).get();
+
+ giveRightToPost(mailboxId, session);
+ Mail mail = sendSubAddressedMail(false);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, "any"));
+ }
+
+ @Test
+ void shouldNotAddStorageDirectiveWhenNooneHasRightAndSenderIsUnknown() throws Exception {
+
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
+
+ // create Mailbox
+ MailboxId mailboxId = mailboxManager.createMailbox(
+ MailboxPath.forUser(username, "any"), session).get();
- Mail mail = sendSubAddressedMail();
+ giveRightToPost(mailboxId, session);
+ Mail mail = sendSubAddressedMail(false);
AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
assertThat(mail.attributes().map(this::unbox))
.doesNotContain(Pair.of(recipient, "any"));
}
- private Mail sendSubAddressedMail() throws MessagingException, IOException {
+ private void giveRightToPost(MailboxId mailboxId, MailboxSession session) throws MailboxException {
+ MailboxACL.ACLCommand command = MailboxACL.command()
+ .key(MailboxACL.ANYBODY_KEY)
+ .rights(MailboxACL.Right.Post)
+ .asAddition();
+
+ mailboxManager.applyRightsCommand(mailboxId, command, session);
+ }
+
+ private Mail sendSubAddressedMail(boolean knownSender) throws MessagingException, IOException {
Review Comment:
Protip: just return a FakeMail.Builder without the sender, and let caller decide if he wants sender or not and let him build the mail then execute the mailbet on it.
Caller code:
```
Mail mail = mailBuilder().sender(...).build();
testee.service(mail);
```
Which is more readable (tests shall be as straightforward as possible and avoid ccidental indirection that obfuscate them...
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1764059733
##########
server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/SubAddressingTest.java:
##########
@@ -96,40 +97,79 @@ void shouldNotAddStorageDirectiveWhenNoRight() throws Exception {
@Test
void shouldAddStorageDirectiveMatchingDetailsWhenRight() throws Exception {
- MailboxSession session = mailboxManager.createSystemSession(Username.of("recipient"));
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
// create Mailbox
MailboxId mailboxId = mailboxManager.createMailbox(
- MailboxPath.forUser(Username.of("recipient"), "any"), session)
- .get();
+ MailboxPath.forUser(username, "any"), session).get();
- // give right to post
- MailboxACL.ACLCommand command = MailboxACL.command()
- .key(MailboxACL.ANYBODY_KEY)
- .rights(MailboxACL.Right.Post)
- .asAddition();
+ giveRightToPost(mailboxId, session);
- mailboxManager.applyRightsCommand(mailboxId, command, session);
+ Mail mail = sendSubAddressedMail(true);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, "any"));
+ }
+
+ @Test
+ void shouldAddStorageDirectiveWhenEveryoneHasRightAndSenderIsUnknown() throws Exception {
+
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
+
+ // create Mailbox
+ MailboxId mailboxId = mailboxManager.createMailbox(
+ MailboxPath.forUser(username, "any"), session).get();
+
+ giveRightToPost(mailboxId, session);
+ Mail mail = sendSubAddressedMail(false);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, "any"));
+ }
+
+ @Test
+ void shouldNotAddStorageDirectiveWhenNooneHasRightAndSenderIsUnknown() throws Exception {
+
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
+
+ // create Mailbox
+ MailboxId mailboxId = mailboxManager.createMailbox(
+ MailboxPath.forUser(username, "any"), session).get();
- Mail mail = sendSubAddressedMail();
+ giveRightToPost(mailboxId, session);
+ Mail mail = sendSubAddressedMail(false);
AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
assertThat(mail.attributes().map(this::unbox))
.doesNotContain(Pair.of(recipient, "any"));
}
- private Mail sendSubAddressedMail() throws MessagingException, IOException {
+ private void giveRightToPost(MailboxId mailboxId, MailboxSession session) throws MailboxException {
+ MailboxACL.ACLCommand command = MailboxACL.command()
+ .key(MailboxACL.ANYBODY_KEY)
+ .rights(MailboxACL.Right.Post)
+ .asAddition();
+
+ mailboxManager.applyRightsCommand(mailboxId, command, session);
+ }
+
+ private Mail sendSubAddressedMail(boolean knownSender) throws MessagingException, IOException {
Review Comment:
boolean parameter is a code smell and proves things likely need a redesign
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1752199863
##########
examples/custom-james-assembly/custom-james-assembly.iml:
##########
@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="UTF-8"?>
Review Comment:
Please configure a local gitignore onto your laptop so that you do not commit ide file into an Apache project
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1757147150
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -0,0 +1,106 @@
+/****************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one *
+ * or more contributor license agreements. See the NOTICE file *
+ * distributed with this work for additional information *
+ * regarding copyright ownership. The ASF licenses this file *
+ * to you under the Apache License, Version 2.0 (the *
+ * "License"); you may not use this file except in compliance *
+ * with the License. You may obtain a copy of the License at *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
+ * Unless required by applicable law or agreed to in writing, *
+ * software distributed under the License is distributed on an *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
+ * KIND, either express or implied. See the License for the *
+ * specific language governing permissions and limitations *
+ * under the License. *
+ ****************************************************************/
+
+package org.apache.james.transport.mailets;
+
+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.MessageManager;
+import org.apache.james.mailbox.exception.MailboxException;
+import org.apache.james.mailbox.exception.MailboxNotFoundException;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxConstants;
+import org.apache.james.mailbox.model.MailboxPath;
+import org.apache.james.metrics.api.MetricFactory;
+import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.api.UsersRepositoryException;
+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;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collections;
+
+import static org.apache.james.mailbox.MessageManager.MailboxMetaData.RecentMode.IGNORE;
+
+public class SubAddressing extends GenericMailet {
+ private final UsersRepository usersRepository;
+ private final MailboxManager mailboxManager;
+ private final MetricFactory metricFactory;
+
+ private static final Logger LOG = LoggerFactory.getLogger(SubAddressing.class);
+
+ @Inject
+ public SubAddressing(UsersRepository usersRepository, @Named("mailboxmanager") MailboxManager mailboxManager, MetricFactory metricFactory) {
+ this.usersRepository = usersRepository;
+ this.mailboxManager = mailboxManager;
+ this.metricFactory = metricFactory;
+ }
+
+ @Override
+ public void service(Mail mail) throws MessagingException {
+ mail.getRecipients().forEach(recipient ->
+ recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
+ .ifPresent(
+ Throwing.consumer(targetFolder -> postIfHasRight(mail, recipient, targetFolder))
+ )
+ );
+ }
+
+ private void postIfHasRight(Mail mail, MailAddress recipient, String targetFolder) throws UsersRepositoryException {
+ if(hasPostRight(recipient, targetFolder)) {
+ StorageDirective.builder().targetFolders(ImmutableList.of(targetFolder)).build()
+ .encodeAsAttributes(usersRepository.getUsername(recipient))
+ .forEach(mail::setAttribute);
+ } else {
+ LOG.info("{} tried to address {}'s subfolder `{}` but did not have the right to", mail.getMaybeSender().toString(), recipient, targetFolder);
+ }
+ }
+
+ private boolean hasPostRight(MailAddress recipient, String targetFolder) {
+
+ try {
+ Username recipientUsername = Username.fromLocalPartWithoutDomain(recipient.getLocalPartWithoutDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER));
+ MailboxSession session = mailboxManager.createSystemSession(recipientUsername);
+
+ MailboxACL aclCommand = mailboxManager
+ .getMailbox(MailboxPath.forUser(recipientUsername, targetFolder), session)
+ .getMetaData(IGNORE, session, MessageManager.MailboxMetaData.FetchGroup.NO_COUNT)
+ .getACL();
+
+ return aclCommand.getEntries().containsKey(MailboxACL.ANYBODY_KEY)
+ && aclCommand.getEntries()
+ .get(MailboxACL.ANYBODY_KEY)
Review Comment:
Use `Optional` to wrap null case, transform your optional, and just call `aclCommand.getEntries().get(MailboxACL.ANYBODY_KEY)` once?
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "florentos17 (via GitHub)" <gi...@apache.org>.
florentos17 commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1758530892
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -0,0 +1,106 @@
+/****************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one *
+ * or more contributor license agreements. See the NOTICE file *
+ * distributed with this work for additional information *
+ * regarding copyright ownership. The ASF licenses this file *
+ * to you under the Apache License, Version 2.0 (the *
+ * "License"); you may not use this file except in compliance *
+ * with the License. You may obtain a copy of the License at *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
+ * Unless required by applicable law or agreed to in writing, *
+ * software distributed under the License is distributed on an *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
+ * KIND, either express or implied. See the License for the *
+ * specific language governing permissions and limitations *
+ * under the License. *
+ ****************************************************************/
+
+package org.apache.james.transport.mailets;
+
+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.MessageManager;
+import org.apache.james.mailbox.exception.MailboxException;
+import org.apache.james.mailbox.exception.MailboxNotFoundException;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxConstants;
+import org.apache.james.mailbox.model.MailboxPath;
+import org.apache.james.metrics.api.MetricFactory;
+import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.api.UsersRepositoryException;
+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;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collections;
+
+import static org.apache.james.mailbox.MessageManager.MailboxMetaData.RecentMode.IGNORE;
+
+public class SubAddressing extends GenericMailet {
+ private final UsersRepository usersRepository;
+ private final MailboxManager mailboxManager;
+ private final MetricFactory metricFactory;
+
+ private static final Logger LOG = LoggerFactory.getLogger(SubAddressing.class);
+
+ @Inject
+ public SubAddressing(UsersRepository usersRepository, @Named("mailboxmanager") MailboxManager mailboxManager, MetricFactory metricFactory) {
+ this.usersRepository = usersRepository;
+ this.mailboxManager = mailboxManager;
+ this.metricFactory = metricFactory;
+ }
+
+ @Override
+ public void service(Mail mail) throws MessagingException {
+ mail.getRecipients().forEach(recipient ->
+ recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
+ .ifPresent(
+ Throwing.consumer(targetFolder -> postIfHasRight(mail, recipient, targetFolder))
+ )
+ );
+ }
+
+ private void postIfHasRight(Mail mail, MailAddress recipient, String targetFolder) throws UsersRepositoryException {
+ if(hasPostRight(recipient, targetFolder)) {
+ StorageDirective.builder().targetFolders(ImmutableList.of(targetFolder)).build()
+ .encodeAsAttributes(usersRepository.getUsername(recipient))
+ .forEach(mail::setAttribute);
+ } else {
+ LOG.info("{} tried to address {}'s subfolder `{}` but did not have the right to", mail.getMaybeSender().toString(), recipient, targetFolder);
+ }
+ }
+
+ private boolean hasPostRight(MailAddress recipient, String targetFolder) {
+
+ try {
+ Username recipientUsername = Username.fromLocalPartWithoutDomain(recipient.getLocalPartWithoutDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER));
+ MailboxSession session = mailboxManager.createSystemSession(recipientUsername);
+
+ MailboxACL aclCommand = mailboxManager
+ .getMailbox(MailboxPath.forUser(recipientUsername, targetFolder), session)
+ .getMetaData(IGNORE, session, MessageManager.MailboxMetaData.FetchGroup.NO_COUNT)
+ .getACL();
+
+ return aclCommand.getEntries().containsKey(MailboxACL.ANYBODY_KEY)
+ && aclCommand.getEntries()
+ .get(MailboxACL.ANYBODY_KEY)
+ .equals(MailboxACL.Rfc4314Rights.of(Collections.singletonList(MailboxACL.Right.Post)));
+
+ } catch (MailboxException e) {
+ return false;
Review Comment:
forwarding of course... :)
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "florentos17 (via GitHub)" <gi...@apache.org>.
florentos17 commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1754976317
##########
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:
1. `?` operator & 2. MailboxContants.INBOX -> done
3 don't do anything if no right & 4. INFO log are still todo
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1757148615
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -0,0 +1,106 @@
+/****************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one *
+ * or more contributor license agreements. See the NOTICE file *
+ * distributed with this work for additional information *
+ * regarding copyright ownership. The ASF licenses this file *
+ * to you under the Apache License, Version 2.0 (the *
+ * "License"); you may not use this file except in compliance *
+ * with the License. You may obtain a copy of the License at *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
+ * Unless required by applicable law or agreed to in writing, *
+ * software distributed under the License is distributed on an *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
+ * KIND, either express or implied. See the License for the *
+ * specific language governing permissions and limitations *
+ * under the License. *
+ ****************************************************************/
+
+package org.apache.james.transport.mailets;
+
+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.MessageManager;
+import org.apache.james.mailbox.exception.MailboxException;
+import org.apache.james.mailbox.exception.MailboxNotFoundException;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxConstants;
+import org.apache.james.mailbox.model.MailboxPath;
+import org.apache.james.metrics.api.MetricFactory;
+import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.api.UsersRepositoryException;
+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;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collections;
+
+import static org.apache.james.mailbox.MessageManager.MailboxMetaData.RecentMode.IGNORE;
+
+public class SubAddressing extends GenericMailet {
+ private final UsersRepository usersRepository;
+ private final MailboxManager mailboxManager;
+ private final MetricFactory metricFactory;
+
+ private static final Logger LOG = LoggerFactory.getLogger(SubAddressing.class);
+
+ @Inject
+ public SubAddressing(UsersRepository usersRepository, @Named("mailboxmanager") MailboxManager mailboxManager, MetricFactory metricFactory) {
+ this.usersRepository = usersRepository;
+ this.mailboxManager = mailboxManager;
+ this.metricFactory = metricFactory;
+ }
+
+ @Override
+ public void service(Mail mail) throws MessagingException {
+ mail.getRecipients().forEach(recipient ->
+ recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
+ .ifPresent(
+ Throwing.consumer(targetFolder -> postIfHasRight(mail, recipient, targetFolder))
+ )
+ );
+ }
+
+ private void postIfHasRight(Mail mail, MailAddress recipient, String targetFolder) throws UsersRepositoryException {
+ if(hasPostRight(recipient, targetFolder)) {
+ StorageDirective.builder().targetFolders(ImmutableList.of(targetFolder)).build()
+ .encodeAsAttributes(usersRepository.getUsername(recipient))
+ .forEach(mail::setAttribute);
+ } else {
+ LOG.info("{} tried to address {}'s subfolder `{}` but did not have the right to", mail.getMaybeSender().toString(), recipient, targetFolder);
+ }
+ }
+
+ private boolean hasPostRight(MailAddress recipient, String targetFolder) {
+
+ try {
+ Username recipientUsername = Username.fromLocalPartWithoutDomain(recipient.getLocalPartWithoutDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER));
+ MailboxSession session = mailboxManager.createSystemSession(recipientUsername);
+
+ MailboxACL aclCommand = mailboxManager
+ .getMailbox(MailboxPath.forUser(recipientUsername, targetFolder), session)
+ .getMetaData(IGNORE, session, MessageManager.MailboxMetaData.FetchGroup.NO_COUNT)
+ .getACL();
+
+ return aclCommand.getEntries().containsKey(MailboxACL.ANYBODY_KEY)
+ && aclCommand.getEntries()
+ .get(MailboxACL.ANYBODY_KEY)
+ .equals(MailboxACL.Rfc4314Rights.of(Collections.singletonList(MailboxACL.Right.Post)));
Review Comment:
What if Rfc4314Rights contains other rights in addition to 'p'? Shall we allow to post in this case?
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on PR #2405:
URL: https://github.com/apache/james-project/pull/2405#issuecomment-2352160764
Using functionnal constructs we can get it slightly cleaner:
```
mail.getMaybeSender().map(usersrepository::getUsernameForMailAddress).orElse(null);
```
Which is slightly better (use UsersRepository to convert MailAddress to Username!)
> What do you suggest ? I could adapt resolveRight() to accept Optional.empty() instead of null, but is it my job to do it ?
I Am not fan either to be honest: null as a parameter for `resolveRights` usage being working seems mostly accidental...
- Do the current code base currently uses "null" to denote absence of user or "anybody"?
- Do the current code base actively test this specific value and specifies clearly the overall behaviour?
As none of these answer is negative we likely cannot rely on this 'null' parameter.
Once again 'p' implies resolving rights as part of the delivery context which is by essence different of applying rights directly linked to user actions. Hence having 'SubAddressing feature bridging the gap seems reasonable to me.
In pseudo code this is what I would write:
```
MailboxACL.Rfc4314Rights rights = mail.getMaybeSender().map(usersrepository::getUsernameForMailAddress)
.map(resolveUserRights)
.orElseGet(getAnyBodyRights); // read directly the ACL for null sender
```
What I also really do not like is the third parameter being an untyped String and not a stronger type.... Like `Username`. We *could* get one commit to address this.
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1758815891
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -19,33 +19,86 @@
package org.apache.james.transport.mailets;
-import com.github.fge.lambdas.Throwing;
-import com.google.common.collect.ImmutableList;
+import static org.apache.james.mailbox.MessageManager.MailboxMetaData.RecentMode.IGNORE;
+
+import java.util.Optional;
+
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.MessageManager;
+import org.apache.james.mailbox.exception.MailboxException;
+import org.apache.james.mailbox.exception.MailboxNotFoundException;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxPath;
import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.api.UsersRepositoryException;
import org.apache.mailet.Mail;
import org.apache.mailet.StorageDirective;
import org.apache.mailet.base.GenericMailet;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+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 static final Logger LOG = LoggerFactory.getLogger(SubAddressing.class);
@Inject
- public SubAddressing(UsersRepository usersRepository) {
+ public SubAddressing(UsersRepository usersRepository, @Named("mailboxmanager") MailboxManager mailboxManager) {
this.usersRepository = usersRepository;
+ this.mailboxManager = mailboxManager;
}
@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))
- ));
+ recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
+ .ifPresent(Throwing.consumer(targetFolder -> postIfHasRight(mail, recipient, targetFolder))));
Review Comment:
postIfHasRight is a brittle method name: it embeds 2 meaning, a condition and an action.
Likely we can avoid that by leveraging functional semantic:
```
recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
.filter(targetFolder -> hasPostRight(recipient, targetFolder))
.ifPresentOrElse(Throwing.consumer(targetFolder -> post(mail, recipient, targetFolder)),
() -> LOG.info("{} tried to address {}'s subfolder `{}` but did not have the right to",
mail.getMaybeSender().toString(), recipient, targetFolder);));
```
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1758815891
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -19,33 +19,86 @@
package org.apache.james.transport.mailets;
-import com.github.fge.lambdas.Throwing;
-import com.google.common.collect.ImmutableList;
+import static org.apache.james.mailbox.MessageManager.MailboxMetaData.RecentMode.IGNORE;
+
+import java.util.Optional;
+
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.MessageManager;
+import org.apache.james.mailbox.exception.MailboxException;
+import org.apache.james.mailbox.exception.MailboxNotFoundException;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxPath;
import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.api.UsersRepositoryException;
import org.apache.mailet.Mail;
import org.apache.mailet.StorageDirective;
import org.apache.mailet.base.GenericMailet;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+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 static final Logger LOG = LoggerFactory.getLogger(SubAddressing.class);
@Inject
- public SubAddressing(UsersRepository usersRepository) {
+ public SubAddressing(UsersRepository usersRepository, @Named("mailboxmanager") MailboxManager mailboxManager) {
this.usersRepository = usersRepository;
+ this.mailboxManager = mailboxManager;
}
@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))
- ));
+ recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
+ .ifPresent(Throwing.consumer(targetFolder -> postIfHasRight(mail, recipient, targetFolder))));
Review Comment:
postIfHasRight is a brittle method name: it embeds 2 meaning, a condition and an action.
Likely we can avoid that by leveraging functional semantic:
```
recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
.filter(targetFolder -> hasPostRight(recipient, targetFolder))
.ifPresent(Throwing.consumer(targetFolder -> post(mail, recipient, targetFolder))));
```
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1766771284
##########
docs/modules/servers/partials/SubAddressing.adoc:
##########
@@ -0,0 +1,22 @@
+=== SubAddressing
Review Comment:
Yes
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1752189685
##########
server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/SubAddressingTest.java:
##########
@@ -0,0 +1,61 @@
+/****************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one *
+ * or more contributor license agreements. See the NOTICE file *
+ * distributed with this work for additional information *
+ * regarding copyright ownership. The ASF licenses this file *
+ * to you under the Apache License, Version 2.0 (the *
+ * "License"); you may not use this file except in compliance *
+ * with the License. You may obtain a copy of the License at *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
+ * Unless required by applicable law or agreed to in writing, *
+ * software distributed under the License is distributed on an *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
+ * KIND, either express or implied. See the License for the *
+ * specific language governing permissions and limitations *
+ * under the License. *
+ ****************************************************************/
+
+package org.apache.james.transport.mailets;
+
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.james.core.MailAddress;
+import org.apache.james.user.memory.MemoryUsersRepository;
+import org.apache.mailet.Attribute;
+import org.apache.mailet.AttributeName;
+import org.apache.mailet.AttributeValue;
+import org.apache.mailet.base.MailAddressFixture;
+import org.apache.mailet.base.test.FakeMail;
+import org.apache.mailet.base.test.FakeMailetConfig;
+import org.junit.jupiter.api.Test;
+
+import java.util.Collection;
+
+import static org.apache.james.transport.mailets.WithStorageDirectiveTest.NO_DOMAIN_LIST;
+import static org.assertj.core.api.Assertions.assertThat;
Review Comment:
Pro tip: configure your import layout cf https://github.com/apache/james-project/blob/8c7ad741a17232f7904d087c4006c21b0a0e5eb1/checkstyle.xml#L188
```
import static ...
import java
import jakarta
import org
import com
import others...
```
(and configure the ide)
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
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!)
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on PR #2405:
URL: https://github.com/apache/james-project/pull/2405#issuecomment-2341249020
` quick checkstyle fixups along with further work on right management for subaddressing - still draft work`
This commit is evil: ideally we shall patch the checkstyle issues separately, ideally as ficup f the commit that introduced them (so that we can squash) and evolutionsshould be onto their own separated commit.
As a result reviewing this commit would be hard for me as there is many unrelated change.
Exercise: Use git rebase to:
- 1. extract the checkstyle fixes and split them from work on rights checking
- 2. squash checkstyle issues with commits that introduced them.
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1757143375
##########
examples/custom-james-assembly/src/main/java/org/apache/james/examples/CustomJamesServerMain.java:
##########
@@ -60,22 +61,29 @@ public class CustomJamesServerMain implements JamesServerMain {
new MemoryMailQueueModule(),
new TaskManagerModule(),
new RawPostDequeueDecoratorModule(),
- binder -> binder.bind(MailetContainerModule.DefaultProcessorsConfigurationSupplier.class)
- .toInstance(BaseHierarchicalConfiguration::new));
+ binder -> binder.bind(
+ MailetContainerModule
+ .DefaultProcessorsConfigurationSupplier.class
+ ).toInstance(BaseHierarchicalConfiguration::new));
public static final Module CUSTOM_SERVER_AGGREGATE_MODULE = Modules.combine(
CUSTOM_SERVER_MODULE,
PROTOCOLS);
public static void main(String[] args) throws Exception {
- MemoryJamesConfiguration configuration = MemoryJamesConfiguration.builder()
+ MemoryJamesConfiguration configuration = MemoryJamesConfiguration
+ .builder()
.useWorkingDirectoryEnvProperty()
.build();
JamesServerMain.main(GuiceJamesServer.forConfiguration(configuration)
.combineWith(CUSTOM_SERVER_AGGREGATE_MODULE)
- .combineWith(new UsersRepositoryModuleChooser(new MemoryUsersRepositoryModule())
- .chooseModules(configuration.getUsersRepositoryImplementation()))
+ .combineWith(new UsersRepositoryModuleChooser(
+ new MemoryUsersRepositoryModule()
+ )
+ .chooseModules(configuration
+ .getUsersRepositoryImplementation()
+ ))
Review Comment:
Idem are those indent change needed?
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "florentos17 (via GitHub)" <gi...@apache.org>.
florentos17 commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1758984076
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -19,33 +19,86 @@
package org.apache.james.transport.mailets;
-import com.github.fge.lambdas.Throwing;
-import com.google.common.collect.ImmutableList;
+import static org.apache.james.mailbox.MessageManager.MailboxMetaData.RecentMode.IGNORE;
+
+import java.util.Optional;
+
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.MessageManager;
+import org.apache.james.mailbox.exception.MailboxException;
+import org.apache.james.mailbox.exception.MailboxNotFoundException;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxPath;
import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.api.UsersRepositoryException;
import org.apache.mailet.Mail;
import org.apache.mailet.StorageDirective;
import org.apache.mailet.base.GenericMailet;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+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 static final Logger LOG = LoggerFactory.getLogger(SubAddressing.class);
@Inject
- public SubAddressing(UsersRepository usersRepository) {
+ public SubAddressing(UsersRepository usersRepository, @Named("mailboxmanager") MailboxManager mailboxManager) {
this.usersRepository = usersRepository;
+ this.mailboxManager = mailboxManager;
}
@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))
- ));
+ recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
+ .ifPresent(Throwing.consumer(targetFolder -> postIfHasRight(mail, recipient, targetFolder))));
Review Comment:
it's not as simple:
1. for the `LOG.info`, `targetFolder` cannot be resolved as the `Else` part is a `Runnable` and not a `Consumer`. We could maybe log a message without specifying the targetFolder, or even remove the log message at all.
2. `service()` cannot throw a MailboxException, so it cannot call `hasPostRight()` which does. I tried to do something like `.filter(ThrowingPredicate(targetFolder -> hasPostRight(mail, recipient, targetFolder)))` but it doesn't work, as targetFolder is no longer considered a String...
would something like this be acceptable ?
```
recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
.ifPresent(Throwing.consumer(targetFolder -> {
if (hasPostRight(mail, recipient, targetFolder)) {
post(mail, recipient, targetFolder);
} else {
LOG.info("{} tried to address {}'s subfolder `{}` but did not have the right to",
mail.getMaybeSender().toString(), recipient, targetFolder);
}
})));
```
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "florentos17 (via GitHub)" <gi...@apache.org>.
florentos17 commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1758984076
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -19,33 +19,86 @@
package org.apache.james.transport.mailets;
-import com.github.fge.lambdas.Throwing;
-import com.google.common.collect.ImmutableList;
+import static org.apache.james.mailbox.MessageManager.MailboxMetaData.RecentMode.IGNORE;
+
+import java.util.Optional;
+
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.MessageManager;
+import org.apache.james.mailbox.exception.MailboxException;
+import org.apache.james.mailbox.exception.MailboxNotFoundException;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxPath;
import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.api.UsersRepositoryException;
import org.apache.mailet.Mail;
import org.apache.mailet.StorageDirective;
import org.apache.mailet.base.GenericMailet;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+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 static final Logger LOG = LoggerFactory.getLogger(SubAddressing.class);
@Inject
- public SubAddressing(UsersRepository usersRepository) {
+ public SubAddressing(UsersRepository usersRepository, @Named("mailboxmanager") MailboxManager mailboxManager) {
this.usersRepository = usersRepository;
+ this.mailboxManager = mailboxManager;
}
@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))
- ));
+ recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
+ .ifPresent(Throwing.consumer(targetFolder -> postIfHasRight(mail, recipient, targetFolder))));
Review Comment:
it's not as simple:
1. for the `LOG.info`, `targetFolder` cannot be resolved as the `Else` part is a `Runnable` and not a `Consumer`. We could maybe log a message without specifying the targetFolder, or even remove the log message at all.
2. `service()` cannot throw a MailboxException, so it cannot call `hasPostRight` which does. I tried to do something like `.filter(ThrowingPredicate(targetFolder -> hasPostRight(mail, recipient, targetFolder)))` but it doesn't work, as targetFolder is no longer considered a String...
would something like this be acceptable ?
```
recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
.ifPresent(Throwing.consumer(targetFolder -> {
if (hasPostRight(mail, recipient, targetFolder)) {
post(mail, recipient, targetFolder);
} else {
LOG.info("{} tried to address {}'s subfolder `{}` but did not have the right to",
mail.getMaybeSender().toString(), recipient, targetFolder);
}
})));
```
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1757156259
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -0,0 +1,106 @@
+/****************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one *
+ * or more contributor license agreements. See the NOTICE file *
+ * distributed with this work for additional information *
+ * regarding copyright ownership. The ASF licenses this file *
+ * to you under the Apache License, Version 2.0 (the *
+ * "License"); you may not use this file except in compliance *
+ * with the License. You may obtain a copy of the License at *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
+ * Unless required by applicable law or agreed to in writing, *
+ * software distributed under the License is distributed on an *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
+ * KIND, either express or implied. See the License for the *
+ * specific language governing permissions and limitations *
+ * under the License. *
+ ****************************************************************/
+
+package org.apache.james.transport.mailets;
+
+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.MessageManager;
+import org.apache.james.mailbox.exception.MailboxException;
+import org.apache.james.mailbox.exception.MailboxNotFoundException;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxConstants;
+import org.apache.james.mailbox.model.MailboxPath;
+import org.apache.james.metrics.api.MetricFactory;
+import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.api.UsersRepositoryException;
+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;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collections;
+
+import static org.apache.james.mailbox.MessageManager.MailboxMetaData.RecentMode.IGNORE;
+
+public class SubAddressing extends GenericMailet {
+ private final UsersRepository usersRepository;
+ private final MailboxManager mailboxManager;
+ private final MetricFactory metricFactory;
+
+ private static final Logger LOG = LoggerFactory.getLogger(SubAddressing.class);
+
+ @Inject
+ public SubAddressing(UsersRepository usersRepository, @Named("mailboxmanager") MailboxManager mailboxManager, MetricFactory metricFactory) {
+ this.usersRepository = usersRepository;
+ this.mailboxManager = mailboxManager;
+ this.metricFactory = metricFactory;
+ }
+
+ @Override
+ public void service(Mail mail) throws MessagingException {
+ mail.getRecipients().forEach(recipient ->
+ recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
+ .ifPresent(
+ Throwing.consumer(targetFolder -> postIfHasRight(mail, recipient, targetFolder))
+ )
+ );
+ }
+
+ private void postIfHasRight(Mail mail, MailAddress recipient, String targetFolder) throws UsersRepositoryException {
+ if(hasPostRight(recipient, targetFolder)) {
Review Comment:
```suggestion
if (hasPostRight(recipient, targetFolder)) {
```
Checkstyle...
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on PR #2405:
URL: https://github.com/apache/james-project/pull/2405#issuecomment-2346698421
(While I have been asking for it privately, ) I feel saddened that we skip right resolution logic alltogether.
Saying that we do not handled setting 'p' rights for external individuals is one things.
But over-simplifying the right resolution logic seems like a mistake.
For instance as off today we would not take into account negative ACLs (every body can port to my mailbox vietnam except Forent). And there would be nothingmore harmfull than thinking you denied Florent that right while actually you did not.
It turns out we can pretty simply, given the MailboxACL resolve the rights independently from the RightManager (which messes around with MailboxSession....)
We could use `MailboxACLResolver`:
```
mailboxACLResolver.resolveRights(sender, acl, recipient).contains(Right.P)
```
Or something like this....
And of course add tests for ACL restrictions...
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1761679161
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -19,33 +19,88 @@
package org.apache.james.transport.mailets;
-import com.github.fge.lambdas.Throwing;
-import com.google.common.collect.ImmutableList;
+import static org.apache.james.mailbox.MessageManager.MailboxMetaData.RecentMode.IGNORE;
+
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.MessageManager;
+import org.apache.james.mailbox.acl.MailboxACLResolver;
+import org.apache.james.mailbox.acl.UnionMailboxACLResolver;
+import org.apache.james.mailbox.exception.MailboxException;
+import org.apache.james.mailbox.exception.MailboxNotFoundException;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxPath;
import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.api.UsersRepositoryException;
import org.apache.mailet.Mail;
import org.apache.mailet.StorageDirective;
import org.apache.mailet.base.GenericMailet;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+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 static final Logger LOG = LoggerFactory.getLogger(SubAddressing.class);
Review Comment:
```
private static final Logger LOG = LoggerFactory.getLogger(SubAddressing.class);
private final UsersRepository usersRepository;
private final MailboxManager mailboxManager;
```
Static fileds go first (convention)
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1763025596
##########
core/src/test/java/org/apache/james/core/MailAddressTest.java:
##########
@@ -225,4 +228,93 @@ void shouldMatchBeanContract() {
EqualsVerifier.forClass(MailAddress.class)
.verify();
}
+
+ @Test
+ void getLocalPartDetailsShouldReturnDetails() throws AddressException {
+ MailAddress mailAddress = new MailAddress("address+details@example.com");
+ assertThat(mailAddress.getLocalPartDetails("+")).contains("details");
+ }
+
+ @Test
+ void getLocalPartDetailsShouldReturnEmptyWhenNone() throws AddressException {
+ MailAddress mailAddress = new MailAddress("address@example.com");
+ assertThat(mailAddress.getLocalPartDetails("+")).isEmpty();
+ }
+
+ @Test
+ void getLocalPartDetailsShouldReturnEmptyWhenEmpty() throws AddressException {
+ MailAddress mailAddress = new MailAddress("address+@example.com");
+ assertThat(mailAddress.getLocalPartDetails("+")).contains("");
+ }
+
+ @Test
+ void getLocalPartDetailsShouldReturnDetailsIncludingOtherOccurencesOfDelimiter() throws AddressException {
+ MailAddress mailAddress = new MailAddress("address+de+tails@example.com");
+ assertThat(mailAddress.getLocalPartDetails("+")).contains("de+tails");
+ }
+
+ @Test
+ void getLocalPartDetailsShouldReturnEmptyWhenAddressStartsWithDelimiter() throws AddressException {
+ MailAddress mailAddress = new MailAddress("+details@example.com");
+ assertThat(mailAddress.getLocalPartDetails("+")).isEmpty();
+ }
+
+ @Test
+ void getLocalPartDetailsShouldReturnDetailsWhenMultipleCharacterDelimiter() throws AddressException {
+ MailAddress mailAddress = new MailAddress("localpart--details@example.com");
+ assertThat(mailAddress.getLocalPartDetails("--")).contains("details");
+ }
+
+ @Test
+ void getLocalPartDetailsShouldBePreciseWithMultipleCharacterDelimiter() throws AddressException {
+ MailAddress mailAddress = new MailAddress("localpart---details@example.com");
+ assertThat(mailAddress.getLocalPartDetails("--")).contains("-details");
+ }
+
+
+
+
+
+ @Test
Review Comment:
```suggestion
}
@Test
```
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "florentos17 (via GitHub)" <gi...@apache.org>.
florentos17 commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1764555858
##########
server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/SubAddressingTest.java:
##########
@@ -96,40 +97,79 @@ void shouldNotAddStorageDirectiveWhenNoRight() throws Exception {
@Test
void shouldAddStorageDirectiveMatchingDetailsWhenRight() throws Exception {
- MailboxSession session = mailboxManager.createSystemSession(Username.of("recipient"));
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
// create Mailbox
MailboxId mailboxId = mailboxManager.createMailbox(
- MailboxPath.forUser(Username.of("recipient"), "any"), session)
- .get();
+ MailboxPath.forUser(username, "any"), session).get();
- // give right to post
- MailboxACL.ACLCommand command = MailboxACL.command()
- .key(MailboxACL.ANYBODY_KEY)
- .rights(MailboxACL.Right.Post)
- .asAddition();
+ giveRightToPost(mailboxId, session);
- mailboxManager.applyRightsCommand(mailboxId, command, session);
+ Mail mail = sendSubAddressedMail(true);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, "any"));
+ }
+
+ @Test
+ void shouldAddStorageDirectiveWhenEveryoneHasRightAndSenderIsUnknown() throws Exception {
+
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
+
+ // create Mailbox
+ MailboxId mailboxId = mailboxManager.createMailbox(
+ MailboxPath.forUser(username, "any"), session).get();
+
+ giveRightToPost(mailboxId, session);
+ Mail mail = sendSubAddressedMail(false);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, "any"));
+ }
+
+ @Test
+ void shouldNotAddStorageDirectiveWhenNooneHasRightAndSenderIsUnknown() throws Exception {
+
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
+
+ // create Mailbox
+ MailboxId mailboxId = mailboxManager.createMailbox(
+ MailboxPath.forUser(username, "any"), session).get();
- Mail mail = sendSubAddressedMail();
+ giveRightToPost(mailboxId, session);
+ Mail mail = sendSubAddressedMail(false);
AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
assertThat(mail.attributes().map(this::unbox))
.doesNotContain(Pair.of(recipient, "any"));
}
- private Mail sendSubAddressedMail() throws MessagingException, IOException {
+ private void giveRightToPost(MailboxId mailboxId, MailboxSession session) throws MailboxException {
Review Comment:
fair point !
the only thing is that initializing the ACLCommand throws an exception so i cannot initialize it as a static final constant directly. so i used a @BeforeAll method to initialize the different commands i will have to use, is that ok ?
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1763033759
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -19,33 +19,86 @@
package org.apache.james.transport.mailets;
-import com.github.fge.lambdas.Throwing;
-import com.google.common.collect.ImmutableList;
+import static org.apache.james.mailbox.MessageManager.MailboxMetaData.RecentMode.IGNORE;
+
+import java.util.Optional;
+
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.MessageManager;
+import org.apache.james.mailbox.exception.MailboxException;
+import org.apache.james.mailbox.exception.MailboxNotFoundException;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxPath;
import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.api.UsersRepositoryException;
import org.apache.mailet.Mail;
import org.apache.mailet.StorageDirective;
import org.apache.mailet.base.GenericMailet;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+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 static final Logger LOG = LoggerFactory.getLogger(SubAddressing.class);
@Inject
- public SubAddressing(UsersRepository usersRepository) {
+ public SubAddressing(UsersRepository usersRepository, @Named("mailboxmanager") MailboxManager mailboxManager) {
this.usersRepository = usersRepository;
+ this.mailboxManager = mailboxManager;
}
@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))
- ));
+ recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
+ .ifPresent(Throwing.consumer(targetFolder -> postIfHasRight(mail, recipient, targetFolder))));
Review Comment:
I suggest looking at this together!
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1764060714
##########
server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/SubAddressingTest.java:
##########
@@ -96,40 +97,79 @@ void shouldNotAddStorageDirectiveWhenNoRight() throws Exception {
@Test
void shouldAddStorageDirectiveMatchingDetailsWhenRight() throws Exception {
- MailboxSession session = mailboxManager.createSystemSession(Username.of("recipient"));
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
// create Mailbox
MailboxId mailboxId = mailboxManager.createMailbox(
- MailboxPath.forUser(Username.of("recipient"), "any"), session)
- .get();
+ MailboxPath.forUser(username, "any"), session).get();
- // give right to post
- MailboxACL.ACLCommand command = MailboxACL.command()
- .key(MailboxACL.ANYBODY_KEY)
- .rights(MailboxACL.Right.Post)
- .asAddition();
+ giveRightToPost(mailboxId, session);
- mailboxManager.applyRightsCommand(mailboxId, command, session);
+ Mail mail = sendSubAddressedMail(true);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, "any"));
+ }
+
+ @Test
+ void shouldAddStorageDirectiveWhenEveryoneHasRightAndSenderIsUnknown() throws Exception {
+
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
+
+ // create Mailbox
+ MailboxId mailboxId = mailboxManager.createMailbox(
+ MailboxPath.forUser(username, "any"), session).get();
+
+ giveRightToPost(mailboxId, session);
+ Mail mail = sendSubAddressedMail(false);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, "any"));
+ }
+
+ @Test
+ void shouldNotAddStorageDirectiveWhenNooneHasRightAndSenderIsUnknown() throws Exception {
+
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
+
+ // create Mailbox
+ MailboxId mailboxId = mailboxManager.createMailbox(
+ MailboxPath.forUser(username, "any"), session).get();
- Mail mail = sendSubAddressedMail();
+ giveRightToPost(mailboxId, session);
+ Mail mail = sendSubAddressedMail(false);
AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
assertThat(mail.attributes().map(this::unbox))
.doesNotContain(Pair.of(recipient, "any"));
}
- private Mail sendSubAddressedMail() throws MessagingException, IOException {
+ private void giveRightToPost(MailboxId mailboxId, MailboxSession session) throws MailboxException {
Review Comment:
If we make MailboxACLCommand a constant then giveRightToPost method can likely be removed as it would add nothing.
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1754961136
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -55,20 +63,36 @@ public void service(Mail mail) throws MessagingException {
.ifPresent(
Throwing.consumer(details -> {
- boolean hasRight = mailboxManager.hasRight(
- MailboxPath.forUser(
- Username.fromMailAddress(recipient.stripDetails("+")),
- details
- ),
- MailboxACL.Right.Post,
- session
- );
+ boolean hasRight;
+
+ try {
+ Username recipientUsername = Username.fromLocalPartWithoutDomain(recipient.getLocalPartWithoutDetails("+"));
+ MailboxSession session = mailboxManager.createSystemSession(recipientUsername);
- String targetFolder = hasRight ? details : "inbox";
+ MailboxACL aclCommand = mailboxManager.getMailbox(
+ MailboxPath.forUser(recipientUsername, details), session)
+ .getMetaData(IGNORE, session, MessageManager.MailboxMetaData.FetchGroup.NO_COUNT)
+ .getACL();
+ hasRight = aclCommand.getEntries().containsKey(MailboxACL.ANYBODY_KEY) &&
+ aclCommand.getEntries().get(MailboxACL.ANYBODY_KEY).equals(MailboxACL.Rfc4314Rights.of(Collections.singletonList(MailboxACL.Right.Post)));
+
+ } catch (MailboxNotFoundException e) {
+ hasRight = false;
+ }
+
+ String targetFolder;
+ if(hasRight) {
+ targetFolder = details;
Review Comment:
method extraction instead?
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1752204650
##########
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
+ );
Review Comment:
Personal taste here but I am at war with lonely ) that takes a signle line.
Its not compact and visually aweful.
Prefer:
```
MailboxPath.forUser(
Username.fromMailAddress(recipient.stripDetails("+")),
details),
```
Can you adapt the code accordingly?
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1757154760
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -0,0 +1,106 @@
+/****************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one *
+ * or more contributor license agreements. See the NOTICE file *
+ * distributed with this work for additional information *
+ * regarding copyright ownership. The ASF licenses this file *
+ * to you under the Apache License, Version 2.0 (the *
+ * "License"); you may not use this file except in compliance *
+ * with the License. You may obtain a copy of the License at *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
+ * Unless required by applicable law or agreed to in writing, *
+ * software distributed under the License is distributed on an *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
+ * KIND, either express or implied. See the License for the *
+ * specific language governing permissions and limitations *
+ * under the License. *
+ ****************************************************************/
+
+package org.apache.james.transport.mailets;
+
+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.MessageManager;
+import org.apache.james.mailbox.exception.MailboxException;
+import org.apache.james.mailbox.exception.MailboxNotFoundException;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxConstants;
+import org.apache.james.mailbox.model.MailboxPath;
+import org.apache.james.metrics.api.MetricFactory;
+import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.api.UsersRepositoryException;
+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;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collections;
+
+import static org.apache.james.mailbox.MessageManager.MailboxMetaData.RecentMode.IGNORE;
+
+public class SubAddressing extends GenericMailet {
+ private final UsersRepository usersRepository;
+ private final MailboxManager mailboxManager;
+ private final MetricFactory metricFactory;
+
+ private static final Logger LOG = LoggerFactory.getLogger(SubAddressing.class);
+
+ @Inject
+ public SubAddressing(UsersRepository usersRepository, @Named("mailboxmanager") MailboxManager mailboxManager, MetricFactory metricFactory) {
+ this.usersRepository = usersRepository;
+ this.mailboxManager = mailboxManager;
+ this.metricFactory = metricFactory;
+ }
+
+ @Override
+ public void service(Mail mail) throws MessagingException {
+ mail.getRecipients().forEach(recipient ->
+ recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
+ .ifPresent(
+ Throwing.consumer(targetFolder -> postIfHasRight(mail, recipient, targetFolder))
+ )
+ );
+ }
+
+ private void postIfHasRight(Mail mail, MailAddress recipient, String targetFolder) throws UsersRepositoryException {
+ if(hasPostRight(recipient, targetFolder)) {
+ StorageDirective.builder().targetFolders(ImmutableList.of(targetFolder)).build()
+ .encodeAsAttributes(usersRepository.getUsername(recipient))
+ .forEach(mail::setAttribute);
+ } else {
+ LOG.info("{} tried to address {}'s subfolder `{}` but did not have the right to", mail.getMaybeSender().toString(), recipient, targetFolder);
+ }
+ }
+
+ private boolean hasPostRight(MailAddress recipient, String targetFolder) {
+
+ try {
+ Username recipientUsername = Username.fromLocalPartWithoutDomain(recipient.getLocalPartWithoutDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER));
+ MailboxSession session = mailboxManager.createSystemSession(recipientUsername);
+
+ MailboxACL aclCommand = mailboxManager
+ .getMailbox(MailboxPath.forUser(recipientUsername, targetFolder), session)
+ .getMetaData(IGNORE, session, MessageManager.MailboxMetaData.FetchGroup.NO_COUNT)
+ .getACL();
+
+ return aclCommand.getEntries().containsKey(MailboxACL.ANYBODY_KEY)
+ && aclCommand.getEntries()
+ .get(MailboxACL.ANYBODY_KEY)
+ .equals(MailboxACL.Rfc4314Rights.of(Collections.singletonList(MailboxACL.Right.Post)));
+
+ } catch (MailboxException e) {
+ return false;
Review Comment:
Error handling is hard...
Problem 1: we are silenting potential problems here. It's bad! We likely shall log something when recovering from an exception.
Problem 2: recovering makes sense when facing MailboxNotFoundException -> OK
But mailbox exception is wider than this and includes other class of issue. Would it make sense to also recover from those? Or would ot be better to forward issues for problem we do not understand so that they get later retried?
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1758820178
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -19,33 +19,86 @@
package org.apache.james.transport.mailets;
-import com.github.fge.lambdas.Throwing;
-import com.google.common.collect.ImmutableList;
+import static org.apache.james.mailbox.MessageManager.MailboxMetaData.RecentMode.IGNORE;
+
+import java.util.Optional;
+
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.MessageManager;
+import org.apache.james.mailbox.exception.MailboxException;
+import org.apache.james.mailbox.exception.MailboxNotFoundException;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxPath;
import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.api.UsersRepositoryException;
import org.apache.mailet.Mail;
import org.apache.mailet.StorageDirective;
import org.apache.mailet.base.GenericMailet;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+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 static final Logger LOG = LoggerFactory.getLogger(SubAddressing.class);
Review Comment:
Static fileds go first (convention)
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "florentos17 (via GitHub)" <gi...@apache.org>.
florentos17 commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1758984076
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -19,33 +19,86 @@
package org.apache.james.transport.mailets;
-import com.github.fge.lambdas.Throwing;
-import com.google.common.collect.ImmutableList;
+import static org.apache.james.mailbox.MessageManager.MailboxMetaData.RecentMode.IGNORE;
+
+import java.util.Optional;
+
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.MessageManager;
+import org.apache.james.mailbox.exception.MailboxException;
+import org.apache.james.mailbox.exception.MailboxNotFoundException;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxPath;
import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.api.UsersRepositoryException;
import org.apache.mailet.Mail;
import org.apache.mailet.StorageDirective;
import org.apache.mailet.base.GenericMailet;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+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 static final Logger LOG = LoggerFactory.getLogger(SubAddressing.class);
@Inject
- public SubAddressing(UsersRepository usersRepository) {
+ public SubAddressing(UsersRepository usersRepository, @Named("mailboxmanager") MailboxManager mailboxManager) {
this.usersRepository = usersRepository;
+ this.mailboxManager = mailboxManager;
}
@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))
- ));
+ recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
+ .ifPresent(Throwing.consumer(targetFolder -> postIfHasRight(mail, recipient, targetFolder))));
Review Comment:
it's not as simple:
1. for the `LOG.info`, `targetFolder` cannot be resolved as the `Else` part is a `Runnable` and not a `Consumer`. We could maybe log a message without specifying the targetFolder, or even remove the log message at all.
2. `service()` cannot throw a MailboxException, so it cannot call `hasPostRight()` which does. I tried to do something like `.filter(ThrowingPredicate(targetFolder -> hasPostRight(mail, recipient, targetFolder)))` but it doesn't work, as targetFolder is no longer considered a String...
to fix these issues, I've come up with:
```
recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
.ifPresent(Throwing.consumer(targetFolder -> {
if (hasPostRight(mail, recipient, targetFolder)) {
post(mail, recipient, targetFolder);
} else {
LOG.info("{} tried to address {}'s subfolder `{}` but did not have the right to",
mail.getMaybeSender().toString(), recipient, targetFolder);
}
})));
```
but it's a mix of Streams and if/else... which makes me want to do a method extraction, and that would bring us right back at the start... what do you suggest ?
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "florentos17 (via GitHub)" <gi...@apache.org>.
florentos17 commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1758984076
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -19,33 +19,86 @@
package org.apache.james.transport.mailets;
-import com.github.fge.lambdas.Throwing;
-import com.google.common.collect.ImmutableList;
+import static org.apache.james.mailbox.MessageManager.MailboxMetaData.RecentMode.IGNORE;
+
+import java.util.Optional;
+
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.MessageManager;
+import org.apache.james.mailbox.exception.MailboxException;
+import org.apache.james.mailbox.exception.MailboxNotFoundException;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxPath;
import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.api.UsersRepositoryException;
import org.apache.mailet.Mail;
import org.apache.mailet.StorageDirective;
import org.apache.mailet.base.GenericMailet;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+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 static final Logger LOG = LoggerFactory.getLogger(SubAddressing.class);
@Inject
- public SubAddressing(UsersRepository usersRepository) {
+ public SubAddressing(UsersRepository usersRepository, @Named("mailboxmanager") MailboxManager mailboxManager) {
this.usersRepository = usersRepository;
+ this.mailboxManager = mailboxManager;
}
@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))
- ));
+ recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
+ .ifPresent(Throwing.consumer(targetFolder -> postIfHasRight(mail, recipient, targetFolder))));
Review Comment:
it's not as simple:
1. for the `LOG.info`, `targetFolder` cannot be resolved as the `Else` part is a `Runnable` and not a `Consumer`. We could maybe log a message without specifying the targetFolder, or even remove the log message at all.
2. `service()` cannot throw a MailboxException, so it cannot call `hasPostRight()` which does. I tried to do something like `.filter(ThrowingPredicate(targetFolder -> hasPostRight(mail, recipient, targetFolder)))` but it doesn't work, as targetFolder is no longer considered a String...
to fix these issues, I've come up with:
```
recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
.ifPresent(Throwing.consumer(targetFolder -> {
if (hasPostRight(mail, recipient, targetFolder)) {
post(mail, recipient, targetFolder);
} else {
LOG.info("{} tried to address {}'s subfolder `{}` but did not have the right to",
mail.getMaybeSender().toString(), recipient, targetFolder);
}
})));
```
but it's a mix of Streams and if/else... which makes me want to do a method extraction, and that would bring us right at the start... what do you suggest ?
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "florentos17 (via GitHub)" <gi...@apache.org>.
florentos17 commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1756397212
##########
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:
INFO log done
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "florentos17 (via GitHub)" <gi...@apache.org>.
florentos17 commented on PR #2405:
URL: https://github.com/apache/james-project/pull/2405#issuecomment-2349206387
> (While I have been asking for it privately, ) I feel saddened that we skip right resolution logic alltogether.
>
> Saying that we do not handled setting 'p' rights for external individuals is one things.
>
> But over-simplifying the right resolution logic seems like a mistake.
>
> For instance as off today we would not take into account negative ACLs (every body can port to my mailbox vietnam except Forent). And there would be nothingmore harmfull than thinking you denied Florent that right while actually you did not.
>
> It turns out we can pretty simply, given the MailboxACL resolve the rights independently from the RightManager (which messes around with MailboxSession....)
>
> We could use `MailboxACLResolver`:
>
> ```
> mailboxACLResolver.resolveRights(sender, acl, recipient).contains(Right.P)
> ```
>
> (It would be anyway cleaner than doing a naive ACL resolution...)
>
> Or something like this....
>
> And of course add tests for ACL restrictions...
---
Using `mailboxACLResolver.resolveRights(sender, acl, recipient)` works very well, except in the case where the MaybeSender is not known. Fortunately, resolveRights can also take `null` as a sender, and works the way we want: if the owner gives the right to everybody, then the right is granted even with a `null` sender.
This results in the following code:
```
Username username;
try {
username = Username.fromMailAddress(mail.getMaybeSender().get());
} catch (NoSuchElementException e) {
username = null;
}
MailboxACL.Rfc4314Rights rights = mailboxACLResolver.resolveRights(
username,
aclCommand,
recipient.toString());
```
This works, but I know we don't like either variable re-allocations nor `null`. What do you suggest ? I could adapt `resolveRight()` to accept `Optional.empty()` instead of `null`, but is it my job to do it ?
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "florentos17 (via GitHub)" <gi...@apache.org>.
florentos17 commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1758521695
##########
server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/SubAddressing.java:
##########
@@ -0,0 +1,106 @@
+/****************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one *
+ * or more contributor license agreements. See the NOTICE file *
+ * distributed with this work for additional information *
+ * regarding copyright ownership. The ASF licenses this file *
+ * to you under the Apache License, Version 2.0 (the *
+ * "License"); you may not use this file except in compliance *
+ * with the License. You may obtain a copy of the License at *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
+ * Unless required by applicable law or agreed to in writing, *
+ * software distributed under the License is distributed on an *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
+ * KIND, either express or implied. See the License for the *
+ * specific language governing permissions and limitations *
+ * under the License. *
+ ****************************************************************/
+
+package org.apache.james.transport.mailets;
+
+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.MessageManager;
+import org.apache.james.mailbox.exception.MailboxException;
+import org.apache.james.mailbox.exception.MailboxNotFoundException;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxConstants;
+import org.apache.james.mailbox.model.MailboxPath;
+import org.apache.james.metrics.api.MetricFactory;
+import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.api.UsersRepositoryException;
+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;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collections;
+
+import static org.apache.james.mailbox.MessageManager.MailboxMetaData.RecentMode.IGNORE;
+
+public class SubAddressing extends GenericMailet {
+ private final UsersRepository usersRepository;
+ private final MailboxManager mailboxManager;
+ private final MetricFactory metricFactory;
+
+ private static final Logger LOG = LoggerFactory.getLogger(SubAddressing.class);
+
+ @Inject
+ public SubAddressing(UsersRepository usersRepository, @Named("mailboxmanager") MailboxManager mailboxManager, MetricFactory metricFactory) {
+ this.usersRepository = usersRepository;
+ this.mailboxManager = mailboxManager;
+ this.metricFactory = metricFactory;
+ }
+
+ @Override
+ public void service(Mail mail) throws MessagingException {
+ mail.getRecipients().forEach(recipient ->
+ recipient.getLocalPartDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER)
+ .ifPresent(
+ Throwing.consumer(targetFolder -> postIfHasRight(mail, recipient, targetFolder))
+ )
+ );
+ }
+
+ private void postIfHasRight(Mail mail, MailAddress recipient, String targetFolder) throws UsersRepositoryException {
+ if(hasPostRight(recipient, targetFolder)) {
+ StorageDirective.builder().targetFolders(ImmutableList.of(targetFolder)).build()
+ .encodeAsAttributes(usersRepository.getUsername(recipient))
+ .forEach(mail::setAttribute);
+ } else {
+ LOG.info("{} tried to address {}'s subfolder `{}` but did not have the right to", mail.getMaybeSender().toString(), recipient, targetFolder);
+ }
+ }
+
+ private boolean hasPostRight(MailAddress recipient, String targetFolder) {
+
+ try {
+ Username recipientUsername = Username.fromLocalPartWithoutDomain(recipient.getLocalPartWithoutDetails(UsersRepository.LOCALPART_DETAIL_DELIMITER));
+ MailboxSession session = mailboxManager.createSystemSession(recipientUsername);
+
+ MailboxACL aclCommand = mailboxManager
+ .getMailbox(MailboxPath.forUser(recipientUsername, targetFolder), session)
+ .getMetaData(IGNORE, session, MessageManager.MailboxMetaData.FetchGroup.NO_COUNT)
+ .getACL();
+
+ return aclCommand.getEntries().containsKey(MailboxACL.ANYBODY_KEY)
+ && aclCommand.getEntries()
+ .get(MailboxACL.ANYBODY_KEY)
+ .equals(MailboxACL.Rfc4314Rights.of(Collections.singletonList(MailboxACL.Right.Post)));
Review Comment:
correct
i replaced `.equals(MailboxACL.Rfc4314Rights.of(Collections.singletonList(MailboxACL.Right.Post)))`
with `.contains(MailboxACL.Right.Post)`
which is far more elegant...
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1765050801
##########
server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/SubAddressingTest.java:
##########
@@ -0,0 +1,213 @@
+/****************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one *
+ * or more contributor license agreements. See the NOTICE file *
+ * distributed with this work for additional information *
+ * regarding copyright ownership. The ASF licenses this file *
+ * to you under the Apache License, Version 2.0 (the *
+ * "License"); you may not use this file except in compliance *
+ * with the License. You may obtain a copy of the License at *
+ * *
+ * http://www.apache.org/licenses/LICENSE-2.0 *
+ * *
+ * Unless required by applicable law or agreed to in writing, *
+ * software distributed under the License is distributed on an *
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY *
+ * KIND, either express or implied. See the License for the *
+ * specific language governing permissions and limitations *
+ * under the License. *
+ ****************************************************************/
+
+package org.apache.james.transport.mailets;
+
+import static org.apache.james.transport.mailets.WithStorageDirectiveTest.NO_DOMAIN_LIST;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Collection;
+
+import jakarta.mail.MessagingException;
+
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.james.core.Username;
+import org.apache.james.mailbox.MailboxManager;
+import org.apache.james.mailbox.MailboxSession;
+import org.apache.james.mailbox.exception.MailboxException;
+import org.apache.james.mailbox.inmemory.manager.InMemoryIntegrationResources;
+import org.apache.james.mailbox.model.MailboxACL;
+import org.apache.james.mailbox.model.MailboxId;
+import org.apache.james.mailbox.model.MailboxPath;
+import org.apache.james.user.api.UsersRepository;
+import org.apache.james.user.memory.MemoryUsersRepository;
+import org.apache.mailet.Attribute;
+import org.apache.mailet.AttributeName;
+import org.apache.mailet.AttributeValue;
+import org.apache.mailet.Mail;
+import org.apache.mailet.base.test.FakeMail;
+import org.apache.mailet.base.test.FakeMailetConfig;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+
+public class SubAddressingTest {
+
+ private static final String SENDER1 = "sender1@localhost";
+ private static final String SENDER2 = "sender2@localhost";
+ private static final String TARGET = "targetfolder";
+ private static final String UNEXISTING_TARGET = "unexistingfolder";
+
+ private MailboxManager mailboxManager;
+ private SubAddressing testee;
+ private UsersRepository usersRepository;
+ private Username sender1Username;
+ private Username sender2Username;
+ private Username recipientUsername;
+ private MailboxSession recipientSession;
+ private MailboxId targetMailboxId;
+
+ @BeforeEach
+ void setUp() throws MailboxException, MessagingException {
+ mailboxManager = InMemoryIntegrationResources.defaultResources().getMailboxManager();
+ usersRepository = MemoryUsersRepository.withVirtualHosting(NO_DOMAIN_LIST);
+
+ testee = new SubAddressing(usersRepository, mailboxManager);
+ testee.init(FakeMailetConfig.builder()
+ .build());
+
+ sender1Username = Username.of("sender1");
+ sender2Username = Username.of("sender2");
+ recipientUsername = Username.of("recipient");
+ recipientSession = mailboxManager.createSystemSession(recipientUsername);
+ targetMailboxId = mailboxManager.createMailbox(
+ MailboxPath.forUser(recipientUsername, TARGET), recipientSession).get();
+ }
+
+ @Test
+ void shouldNotAddStorageDirectiveWhenTargetMailboxDoesNotExist() throws Exception {
+ Mail mail = mailBuilder(UNEXISTING_TARGET).sender(SENDER1).build();
+ testee.service(mail);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, UNEXISTING_TARGET));
+ }
+
+ @Test
+ void shouldNotAddStorageDirectiveWhenNobodyHasRight() throws Exception {
+ givePostRightForKey(MailboxACL.ANYBODY_NEGATIVE_KEY);
+
+ Mail mail = mailBuilder(TARGET).sender(SENDER1).build();
+ testee.service(mail);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, TARGET));
+ }
+
+
+ @Test
+ void shouldAddStorageDirectiveWhenAnybodyHasRight() throws Exception {
+ givePostRightForKey(MailboxACL.ANYBODY_KEY);
+
+ Mail mail = mailBuilder(TARGET).sender(SENDER1).build();
+ testee.service(mail);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .containsOnly(Pair.of(recipient, TARGET));
+ }
+
+ @Test
+ void shouldAddStorageDirectiveWhenSenderIsWhiteListed() throws Exception {
+ // whitelist sender 1 and send from sender 1
+ givePostRightForKey(MailboxACL.EntryKey.createUserEntryKey(sender1Username, false));
+
+ Mail mail = mailBuilder(TARGET).sender(SENDER1).build();
+ testee.service(mail);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .containsOnly(Pair.of(recipient, TARGET));
+ }
+
+ @Test
+ void shouldNotAddStorageDirectiveWhenSenderIsNotWhiteListed() throws Exception {
+ // whitelist sender 1 and send from sender 2
+ givePostRightForKey(MailboxACL.EntryKey.createUserEntryKey(sender1Username, false));
+
+ Mail mail = mailBuilder(TARGET).sender(SENDER2).build();
+ testee.service(mail);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, TARGET));
+ }
+
+ @Test
+ void shouldNotAddStorageDirectiveWhenSenderIsBlackListed() throws Exception {
+ // blacklist sender 1 and send from sender 1
+ givePostRightForKey(MailboxACL.EntryKey.createUserEntryKey(sender1Username, true));
Review Comment:
true is a magic constant. Can't guess easily what it means. Extracting it as a constant could allow naming and help express the intent.
Also we could improve EntryKey api:
```
// API stability
MailboxACL.EntryKey.createUserEntryKey(sender1Username, false|false)
MailboxACL.EntryKey.createUserEntryKey(sender1Username)
MailboxACL.EntryKey.createNegativeUserEntryKey(sender1Username)
```
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa merged PR #2405:
URL: https://github.com/apache/james-project/pull/2405
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1765039157
##########
docs/modules/servers/partials/SubAddressing.adoc:
##########
@@ -0,0 +1,22 @@
+=== SubAddressing
Review Comment:
https://james.staged.apache.org/james-project/3.9.0/servers/distributed/configure/mailets.html
It would be great to have an exemple that can easily get copy pasted
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1764064929
##########
server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/SubAddressingTest.java:
##########
@@ -96,40 +97,79 @@ void shouldNotAddStorageDirectiveWhenNoRight() throws Exception {
@Test
void shouldAddStorageDirectiveMatchingDetailsWhenRight() throws Exception {
- MailboxSession session = mailboxManager.createSystemSession(Username.of("recipient"));
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
// create Mailbox
MailboxId mailboxId = mailboxManager.createMailbox(
- MailboxPath.forUser(Username.of("recipient"), "any"), session)
- .get();
+ MailboxPath.forUser(username, "any"), session).get();
- // give right to post
- MailboxACL.ACLCommand command = MailboxACL.command()
- .key(MailboxACL.ANYBODY_KEY)
- .rights(MailboxACL.Right.Post)
- .asAddition();
+ giveRightToPost(mailboxId, session);
- mailboxManager.applyRightsCommand(mailboxId, command, session);
+ Mail mail = sendSubAddressedMail(true);
+
+ AttributeName recipient = AttributeName.of("DeliveryPaths_recipient@localhost");
+ assertThat(mail.attributes().map(this::unbox))
+ .doesNotContain(Pair.of(recipient, "any"));
+ }
+
+ @Test
+ void shouldAddStorageDirectiveWhenEveryoneHasRightAndSenderIsUnknown() throws Exception {
+
+ Username username = Username.of("recipient");
+ MailboxSession session = mailboxManager.createSystemSession(username);
+
+ // create Mailbox
+ MailboxId mailboxId = mailboxManager.createMailbox(
+ MailboxPath.forUser(username, "any"), session).get();
Review Comment:
I'd just move it in @BeforeEach and handle the case mailbox do not exist with a mailbox called "notfound"
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "florentos17 (via GitHub)" <gi...@apache.org>.
florentos17 commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1765108236
##########
docs/modules/servers/partials/SubAddressing.adoc:
##########
@@ -0,0 +1,22 @@
+=== SubAddressing
Review Comment:
should i just add that ?
```
<mailet match="All" class="SubAddressing">
</mailet>
```
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on PR #2405:
URL: https://github.com/apache/james-project/pull/2405#issuecomment-2341238449
` quick checkstyle fixups along with further work on right management for subaddressing - still draft work`
Ok -bisutage-
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org
Re: [PR] James 3945 [james-project]
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #2405:
URL: https://github.com/apache/james-project/pull/2405#discussion_r1752201906
##########
examples/custom-james-assembly/src/main/java/org/apache/james/examples/CustomJamesServerMain.java:
##########
@@ -44,6 +44,7 @@
import com.google.inject.util.Modules;
public class CustomJamesServerMain implements JamesServerMain {
+
Review Comment:
This example issue shall be adressed onto a separate commit...
--
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: notifications-unsubscribe@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org