This is an automated email from the ASF dual-hosted git repository. btellier pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/james-project.git
commit b505d74da583712543f0507b651d5a74ff8d41ba Author: Gautier DI FOLCO <[email protected]> AuthorDate: Tue Oct 29 13:05:47 2019 +0100 JAMES-2939 Prevent mixed case INBOX creation through JMAP --- .../org/apache/james/mailbox/MailboxManager.java | 13 ++++ .../mailbox/exception/InboxAlreadyCreated.java | 50 ++++++++++++++ .../apache/james/mailbox/MailboxManagerTest.java | 63 ++++++++++++++++++ .../james/mailbox/store/StoreMailboxManager.java | 76 +++++++++++++++------- .../org/apache/james/imap/scripts/StringArgs.test | 6 +- .../integration/SetMailboxesMethodTest.java | 32 +++++++++ .../methods/SetMailboxesCreationProcessor.java | 14 +++- .../james/jmap/draft/model/MailboxFactoryTest.java | 13 ++-- .../james/jmap/draft/utils/MailboxUtilsTest.java | 5 +- 9 files changed, 237 insertions(+), 35 deletions(-) diff --git a/mailbox/api/src/main/java/org/apache/james/mailbox/MailboxManager.java b/mailbox/api/src/main/java/org/apache/james/mailbox/MailboxManager.java index 3af341c..d9bdf61 100644 --- a/mailbox/api/src/main/java/org/apache/james/mailbox/MailboxManager.java +++ b/mailbox/api/src/main/java/org/apache/james/mailbox/MailboxManager.java @@ -256,6 +256,19 @@ public interface MailboxManager extends RequestAware, RightManager, MailboxAnnot boolean mailboxExists(MailboxPath mailboxPath, MailboxSession session) throws MailboxException; /** + * Does the user INBOX exist? + * + * @param session + * the context for this call, not null + * @return true when the INBOX exists and is accessible for the given + * user, false otherwise + * @throws MailboxException + */ + default boolean hasInbox(MailboxSession session) throws MailboxException { + return mailboxExists(MailboxPath.inbox(session), session); + } + + /** * Creates a new system session.<br> * A system session is intended to be used for programmatic access.<br> * Use {@link #login(String, String)} when accessing this API from a diff --git a/mailbox/api/src/main/java/org/apache/james/mailbox/exception/InboxAlreadyCreated.java b/mailbox/api/src/main/java/org/apache/james/mailbox/exception/InboxAlreadyCreated.java new file mode 100644 index 0000000..c673c39 --- /dev/null +++ b/mailbox/api/src/main/java/org/apache/james/mailbox/exception/InboxAlreadyCreated.java @@ -0,0 +1,50 @@ +/** + * ************************************************************* + * 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.mailbox.exception; + +/** + * Indicates that the operation failed since INBOX already exists. + */ +public class InboxAlreadyCreated extends MailboxException { + + private static final long serialVersionUID = -486251759505030366L; + + private final String mailboxName; + + public InboxAlreadyCreated(String mailboxName) { + super("The mailbox '" + mailboxName + "' already exists as 'INBOX'"); + this.mailboxName = mailboxName; + } + + /** + * Gets the name of the mailbox which already exists. + * + * @return the mailboxName, not null + */ + public final String getMailboxName() { + return mailboxName; + } + + public String toString() { + return getMessage(); + } + +} diff --git a/mailbox/api/src/test/java/org/apache/james/mailbox/MailboxManagerTest.java b/mailbox/api/src/test/java/org/apache/james/mailbox/MailboxManagerTest.java index e0a2ad9..eacf95e 100644 --- a/mailbox/api/src/test/java/org/apache/james/mailbox/MailboxManagerTest.java +++ b/mailbox/api/src/test/java/org/apache/james/mailbox/MailboxManagerTest.java @@ -49,7 +49,10 @@ import org.apache.james.mailbox.events.MailboxListener; import org.apache.james.mailbox.events.MessageMoveEvent; import org.apache.james.mailbox.exception.AnnotationException; import org.apache.james.mailbox.exception.HasEmptyMailboxNameInHierarchyException; +import org.apache.james.mailbox.exception.InboxAlreadyCreated; import org.apache.james.mailbox.exception.MailboxException; +import org.apache.james.mailbox.exception.MailboxExistsException; +import org.apache.james.mailbox.exception.MailboxNotFoundException; import org.apache.james.mailbox.exception.TooLongMailboxNameException; import org.apache.james.mailbox.extension.PreDeletionHook; import org.apache.james.mailbox.mock.DataProvisioner; @@ -165,6 +168,66 @@ public abstract class MailboxManagerTest<T extends MailboxManager> { } @Nested + class MailboxCreationTests { + @Test + void hasInboxShouldBeFalseWhenINBOXIsNotCreated() throws Exception { + session = mailboxManager.createSystemSession(USER_1); + mailboxManager.startProcessingRequest(session); + + assertThat(mailboxManager.hasInbox(session)).isFalse(); + } + + @Test + void hasInboxShouldBeTrueWhenINBOXIsCreated() throws Exception { + session = mailboxManager.createSystemSession(USER_1); + mailboxManager.startProcessingRequest(session); + + MailboxPath mailboxPath = MailboxPath.inbox(session); + Optional<MailboxId> mailboxId = mailboxManager.createMailbox(mailboxPath, session); + MessageManager retrievedMailbox = mailboxManager.getMailbox(mailboxPath, session); + + assertThat(mailboxManager.hasInbox(session)).isTrue(); + assertThat(mailboxId.get()).isEqualTo(retrievedMailbox.getId()); + } + + @Test + void creatingMixedCaseINBOXShouldCreateItAsINBOX() throws Exception { + session = mailboxManager.createSystemSession(USER_1); + mailboxManager.startProcessingRequest(session); + + Optional<MailboxId> mailboxId = mailboxManager.createMailbox(MailboxPath.forUser(USER_1, "iNbOx"), session); + MessageManager retrievedMailbox = mailboxManager.getMailbox(MailboxPath.inbox(session), session); + + assertThat(mailboxManager.hasInbox(session)).isTrue(); + assertThat(mailboxId.get()).isEqualTo(retrievedMailbox.getId()); + } + + @Test + void creatingMixedCaseINBOXShouldNotBeRetrievableAsIt() throws Exception { + session = mailboxManager.createSystemSession(USER_1); + mailboxManager.startProcessingRequest(session); + + MailboxPath mailboxPath = MailboxPath.forUser(USER_1, "iNbOx"); + Optional<MailboxId> mailboxId = mailboxManager.createMailbox(mailboxPath, session); + assertThat(mailboxId).isPresent(); + + assertThatThrownBy(() -> mailboxManager.getMailbox(mailboxPath, session)) + .isInstanceOf(MailboxNotFoundException.class); + } + + @Test + void creatingMixedCaseINBOXWhenItHasAlreadyBeenCreatedShouldThrow() throws Exception { + session = mailboxManager.createSystemSession(USER_1); + mailboxManager.startProcessingRequest(session); + + mailboxManager.createMailbox(MailboxPath.inbox(session), session); + + assertThatThrownBy(() -> mailboxManager.createMailbox(MailboxPath.forUser(USER_1, "iNbOx"), session)) + .isInstanceOf(InboxAlreadyCreated.class); + } + } + + @Nested class MailboxNameLimitTests { @Test void creatingMailboxShouldNotFailWhenLimitNameLength() throws Exception { diff --git a/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java b/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java index abd895c..c76ae83 100644 --- a/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java +++ b/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java @@ -44,6 +44,7 @@ import org.apache.james.mailbox.StandardMailboxMetaDataComparator; import org.apache.james.mailbox.events.EventBus; import org.apache.james.mailbox.events.MailboxIdRegistrationKey; import org.apache.james.mailbox.exception.HasEmptyMailboxNameInHierarchyException; +import org.apache.james.mailbox.exception.InboxAlreadyCreated; import org.apache.james.mailbox.exception.InsufficientRightsException; import org.apache.james.mailbox.exception.MailboxException; import org.apache.james.mailbox.exception.MailboxExistsException; @@ -352,34 +353,64 @@ public class StoreMailboxManager implements MailboxManager { return Optional.empty(); } - private List<MailboxId> createMailboxesForPath(MailboxSession mailboxSession, MailboxPath sanitizedMailboxPath) throws MailboxException { + private List<MailboxId> createMailboxesForPath(MailboxSession mailboxSession, MailboxPath sanitizedMailboxPath) { // Create parents first // If any creation fails then the mailbox will not be created // TODO: transaction + List<MailboxPath> intermediatePaths = sanitizedMailboxPath.getHierarchyLevels(getDelimiter()); + boolean isRootPath = intermediatePaths.size() == 1; + + return intermediatePaths + .stream() + .flatMap(Throwing.<MailboxPath, Stream<MailboxId>>function(mailbox -> manageMailboxCreation(mailboxSession, isRootPath, mailbox)).sneakyThrow()) + .collect(Guavate.toImmutableList()); + } + + private Stream<MailboxId> manageMailboxCreation(MailboxSession mailboxSession, boolean isRootPath, MailboxPath mailbox) throws MailboxException { + if (mailbox.isInbox()) { + if (hasInbox(mailboxSession)) { + return duplicatedINBOXCreation(isRootPath, mailbox); + } + + return performConcurrentMailboxCreation(mailboxSession, MailboxPath.inbox(mailboxSession)).stream(); + } + + return performConcurrentMailboxCreation(mailboxSession, mailbox).stream(); + } + + + private Stream<MailboxId> duplicatedINBOXCreation(boolean isRootPath, MailboxPath mailbox) throws InboxAlreadyCreated { + if (isRootPath) { + throw new InboxAlreadyCreated(mailbox.getName()); + } + + return Stream.empty(); + } + + private List<MailboxId> performConcurrentMailboxCreation(MailboxSession mailboxSession, MailboxPath mailbox) throws MailboxException { List<MailboxId> mailboxIds = new ArrayList<>(); - for (MailboxPath mailbox : sanitizedMailboxPath.getHierarchyLevels(getDelimiter())) { - locker.executeWithLock(mailboxSession, mailbox, (LockAwareExecution<Void>) () -> { - if (!mailboxExists(mailbox, mailboxSession)) { - Mailbox m = doCreateMailbox(mailbox); - MailboxMapper mapper = mailboxSessionMapperFactory.getMailboxMapper(mailboxSession); - try { - mapper.execute(Mapper.toTransaction(() -> mailboxIds.add(mapper.save(m)))); - // notify listeners - eventBus.dispatch(EventFactory.mailboxAdded() - .randomEventId() - .mailboxSession(mailboxSession) - .mailbox(m) - .build(), - new MailboxIdRegistrationKey(m.getMailboxId())) - .block(); - } catch (MailboxExistsException e) { - LOGGER.info("{} mailbox was created concurrently", m.generateAssociatedPath()); - } + locker.executeWithLock(mailboxSession, mailbox, (LockAwareExecution<Void>) () -> { + if (!mailboxExists(mailbox, mailboxSession)) { + Mailbox m = doCreateMailbox(mailbox); + MailboxMapper mapper = mailboxSessionMapperFactory.getMailboxMapper(mailboxSession); + try { + mapper.execute(Mapper.toTransaction(() -> mailboxIds.add(mapper.save(m)))); + // notify listeners + eventBus.dispatch(EventFactory.mailboxAdded() + .randomEventId() + .mailboxSession(mailboxSession) + .mailbox(m) + .build(), + new MailboxIdRegistrationKey(m.getMailboxId())) + .block(); + } catch (MailboxExistsException e) { + LOGGER.info("{} mailbox was created concurrently", m.generateAssociatedPath()); } - return null; + } + return null; + + }, true); - }, true); - } return mailboxIds; } @@ -645,7 +676,6 @@ public class StoreMailboxManager implements MailboxManager { } catch (MailboxNotFoundException e) { return false; } - } /** diff --git a/mpt/impl/imap-mailbox/core/src/main/resources/org/apache/james/imap/scripts/StringArgs.test b/mpt/impl/imap-mailbox/core/src/main/resources/org/apache/james/imap/scripts/StringArgs.test index eb81a9a..220a42c 100644 --- a/mpt/impl/imap-mailbox/core/src/main/resources/org/apache/james/imap/scripts/StringArgs.test +++ b/mpt/impl/imap-mailbox/core/src/main/resources/org/apache/james/imap/scripts/StringArgs.test @@ -28,11 +28,15 @@ C: a001 STATUS INBOX (MESSAGES) S: \* STATUS \"INBOX\" \(MESSAGES \d+\) S: a001 OK STATUS completed. -# Case-insensitive inbox +# Case-insensitive INBOX C: a001 STATUS InBoX (MESSAGES) S: \* STATUS \"INBOX\" \(MESSAGES \d+\) S: a001 OK STATUS completed. +# Case-insensitive INBOX creation check +C: a1 CREATE "iNbOx" +S: a1 NO CREATE failed. Mailbox already exists. + # Tests with an atomic mailbox name. C: a1 CREATE atomMailbox S: a1 OK CREATE completed. diff --git a/server/protocols/jmap-draft-integration-testing/jmap-draft-integration-testing-common/src/test/java/org/apache/james/jmap/draft/methods/integration/SetMailboxesMethodTest.java b/server/protocols/jmap-draft-integration-testing/jmap-draft-integration-testing-common/src/test/java/org/apache/james/jmap/draft/methods/integration/SetMailboxesMethodTest.java index 919b640..c55f9d1 100644 --- a/server/protocols/jmap-draft-integration-testing/jmap-draft-integration-testing-common/src/test/java/org/apache/james/jmap/draft/methods/integration/SetMailboxesMethodTest.java +++ b/server/protocols/jmap-draft-integration-testing/jmap-draft-integration-testing-common/src/test/java/org/apache/james/jmap/draft/methods/integration/SetMailboxesMethodTest.java @@ -275,6 +275,38 @@ public abstract class SetMailboxesMethodTest { assertThat(mailboxProbe.listSubscriptions(username)).contains("foo"); } + @Category(BasicFeature.class) + @Test + public void userShouldGetAnErrorWhenNotBeAbleToCreateACaseVariationOfINBOX() throws Exception { + String requestBody = + "[" + + " [ \"setMailboxes\"," + + " {" + + " \"create\": {" + + " \"create-id01\" : {" + + " \"name\" : \"iNbOx\"" + + " }" + + " }" + + " }," + + " \"#0\"" + + " ]" + + "]"; + + given() + .header("Authorization", accessToken.serialize()) + .body(requestBody) + .when() + .post("/jmap") + .then() + .statusCode(200) + .body(NAME, equalTo("mailboxesSet")) + .body(ARGUMENTS + ".notCreated", hasKey("create-id01")) + .body(ARGUMENTS + ".notCreated.create-id01.type", equalTo("invalidArguments")) + .body(ARGUMENTS + ".notCreated.create-id01.description", equalTo("The mailbox 'iNbOx' already exists as 'INBOX'")); + + assertThat(mailboxProbe.listUserMailboxes(username)).doesNotContain("iNbOx"); + } + @Test public void userShouldBeSubscribedOnCreatedMailboxWhenCreateChildOfInboxMailbox() throws Exception { MailboxId inboxId = mailboxProbe.getMailboxId(MailboxConstants.USER_NAMESPACE, username, MailboxConstants.INBOX); diff --git a/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/SetMailboxesCreationProcessor.java b/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/SetMailboxesCreationProcessor.java index 84c0c01..7663c5b 100644 --- a/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/SetMailboxesCreationProcessor.java +++ b/server/protocols/jmap-draft/src/main/java/org/apache/james/jmap/draft/methods/SetMailboxesCreationProcessor.java @@ -41,6 +41,7 @@ import org.apache.james.jmap.draft.utils.SortingHierarchicalCollections; import org.apache.james.mailbox.MailboxManager; import org.apache.james.mailbox.MailboxSession; import org.apache.james.mailbox.SubscriptionManager; +import org.apache.james.mailbox.exception.InboxAlreadyCreated; import org.apache.james.mailbox.exception.MailboxException; import org.apache.james.mailbox.exception.MailboxExistsException; import org.apache.james.mailbox.exception.MailboxNameException; @@ -153,13 +154,20 @@ public class SetMailboxesCreationProcessor implements SetMailboxesProcessor { .type(SetError.Type.INVALID_ARGUMENTS) .description(message) .build()); + } catch (InboxAlreadyCreated e) { + String message = String.format("The mailbox '%s' already exists as 'INBOX'", e.getMailboxName()); + LOGGER.error(message, e); + builder.notCreated(mailboxCreationId, SetError.builder() + .type(SetError.Type.INVALID_ARGUMENTS) + .description(message) + .build()); } catch (MailboxException e) { String message = String.format("An error occurred when creating the mailbox '%s'", mailboxCreationId.getCreationId()); LOGGER.error(message, e); builder.notCreated(mailboxCreationId, SetError.builder() - .type(SetError.Type.ERROR) - .description(message) - .build()); + .type(SetError.Type.ERROR) + .description(message) + .build()); } } diff --git a/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/model/MailboxFactoryTest.java b/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/model/MailboxFactoryTest.java index 36edf82..e507930 100644 --- a/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/model/MailboxFactoryTest.java +++ b/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/model/MailboxFactoryTest.java @@ -29,6 +29,7 @@ import org.apache.james.mailbox.inmemory.InMemoryId; import org.apache.james.mailbox.inmemory.manager.InMemoryIntegrationResources; import org.apache.james.mailbox.manager.ManagerTestProvisionner; import org.apache.james.mailbox.model.MailboxACL; +import org.apache.james.mailbox.model.MailboxConstants; import org.apache.james.mailbox.model.MailboxId; import org.apache.james.mailbox.model.MailboxMetaData; import org.apache.james.mailbox.model.MailboxPath; @@ -132,11 +133,11 @@ public class MailboxFactoryTest { @Test public void getParentIdFromMailboxPathShouldReturnParentIdWhenChildMailbox() throws Exception { - MailboxPath parentMailboxPath = MailboxPath.forUser(user, "inbox"); + MailboxPath parentMailboxPath = MailboxPath.forUser(user, MailboxConstants.INBOX); mailboxManager.createMailbox(parentMailboxPath, mailboxSession); MailboxId parentId = mailboxManager.getMailbox(parentMailboxPath, mailboxSession).getId(); - MailboxPath mailboxPath = MailboxPath.forUser(user, "inbox.mailbox"); + MailboxPath mailboxPath = MailboxPath.forUser(user, "INBOX.mailbox"); mailboxManager.createMailbox(mailboxPath, mailboxSession); Optional<MailboxId> id = sut.getParentIdFromMailboxPath(mailboxPath, Optional.empty(), mailboxSession); @@ -192,7 +193,7 @@ public class MailboxFactoryTest { @Test public void getNamespaceShouldReturnDelegatedNamespaceWhenUserMailboxPathAndUserMailboxSessionAreNotTheSame() throws Exception { - MailboxPath inbox = MailboxPath.forUser(user, "inbox"); + MailboxPath inbox = MailboxPath.forUser(user, MailboxConstants.INBOX); Optional<MailboxId> mailboxId = mailboxManager.createMailbox(inbox, mailboxSession); mailboxManager.applyRightsCommand(inbox, MailboxACL.command() @@ -232,7 +233,7 @@ public class MailboxFactoryTest { @Test public void delegatedUserShouldHaveMayAddItemsWhenAllowedToInsert() throws Exception { - MailboxPath inbox = MailboxPath.forUser(user, "inbox"); + MailboxPath inbox = MailboxPath.forUser(user, MailboxConstants.INBOX); Optional<MailboxId> mailboxId = mailboxManager.createMailbox(inbox, mailboxSession); mailboxManager.applyRightsCommand(inbox, MailboxACL.command() @@ -257,7 +258,7 @@ public class MailboxFactoryTest { @Test public void delegatedUserShouldHaveMayReadItemsWhenAllowedToRead() throws Exception { - MailboxPath inbox = MailboxPath.forUser(user, "inbox"); + MailboxPath inbox = MailboxPath.forUser(user, MailboxConstants.INBOX); Optional<MailboxId> mailboxId = mailboxManager.createMailbox(inbox, mailboxSession); mailboxManager.applyRightsCommand(inbox, MailboxACL.command() @@ -282,7 +283,7 @@ public class MailboxFactoryTest { @Test public void delegatedUserShouldHaveMayRemoveItemsWhenAllowedToRemoveItems() throws Exception { - MailboxPath inbox = MailboxPath.forUser(user, "inbox"); + MailboxPath inbox = MailboxPath.forUser(user, MailboxConstants.INBOX); Optional<MailboxId> mailboxId = mailboxManager.createMailbox(inbox, mailboxSession); mailboxManager.applyRightsCommand(inbox, MailboxACL.command() diff --git a/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/utils/MailboxUtilsTest.java b/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/utils/MailboxUtilsTest.java index 7a48a3a..5269169 100644 --- a/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/utils/MailboxUtilsTest.java +++ b/server/protocols/jmap-draft/src/test/java/org/apache/james/jmap/draft/utils/MailboxUtilsTest.java @@ -24,6 +24,7 @@ import static org.assertj.core.api.Assertions.assertThat; import org.apache.james.mailbox.MailboxManager; import org.apache.james.mailbox.MailboxSession; import org.apache.james.mailbox.inmemory.manager.InMemoryIntegrationResources; +import org.apache.james.mailbox.model.MailboxConstants; import org.apache.james.mailbox.model.MailboxId; import org.apache.james.mailbox.model.MailboxPath; import org.junit.Before; @@ -55,11 +56,11 @@ public class MailboxUtilsTest { @Test public void hasChildrenShouldReturnTrueWhenHasAChild() throws Exception { - MailboxPath parentMailboxPath = MailboxPath.forUser(user, "inbox"); + MailboxPath parentMailboxPath = MailboxPath.forUser(user, MailboxConstants.INBOX); mailboxManager.createMailbox(parentMailboxPath, mailboxSession); MailboxId parentId = mailboxManager.getMailbox(parentMailboxPath, mailboxSession).getId(); - MailboxPath mailboxPath = MailboxPath.forUser(user, "inbox.myBox"); + MailboxPath mailboxPath = MailboxPath.forUser(user, "INBOX.myBox"); mailboxManager.createMailbox(mailboxPath, mailboxSession); assertThat(sut.hasChildren(parentId, mailboxSession)).isTrue(); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
