JAMES-2145 Attachment security, user cannot download attachment of another user
Project: http://git-wip-us.apache.org/repos/asf/james-project/repo Commit: http://git-wip-us.apache.org/repos/asf/james-project/commit/11842e0f Tree: http://git-wip-us.apache.org/repos/asf/james-project/tree/11842e0f Diff: http://git-wip-us.apache.org/repos/asf/james-project/diff/11842e0f Branch: refs/heads/master Commit: 11842e0f2f974e731edf175d7a1b5706e7b9242a Parents: c9ac405 Author: quynhn <qngu...@linagora.com> Authored: Mon Sep 18 18:03:15 2017 +0700 Committer: Antoine Duprat <adup...@linagora.com> Committed: Wed Sep 20 16:18:39 2017 +0200 ---------------------------------------------------------------------- .../apache/james/mailbox/AttachmentManager.java | 2 - .../apache/james/mailbox/MessageIdManager.java | 4 + .../james/mailbox/model/AttachmentId.java | 9 ++- .../james/mailbox/model/AttachmentIdTest.java | 39 +++++++--- .../CassandraMailboxSessionMapperFactory.java | 2 +- .../mail/CassandraMessageIdMapper.java | 5 +- .../InMemoryMailboxSessionMapperFactory.java | 4 +- .../inmemory/InMemoryMessageIdManager.java | 41 ++++++---- .../mailbox/store/StoreAttachmentManager.java | 52 ++++++++----- .../james/mailbox/store/StoreBlobManager.java | 43 +++++------ .../mailbox/store/StoreMessageIdManager.java | 12 +++ .../store/mail/AttachmentMapperFactory.java | 5 +- .../mailbox/store/mail/MessageIdMapper.java | 3 +- .../AbstractMessageIdManagerStorageTest.java | 28 +++++++ .../store/StoreAttachmentManagerTest.java | 80 ++++++++++++++++++++ .../store/TestMailboxSessionMapperFactory.java | 5 +- .../integration/cucumber/DownloadStepdefs.java | 5 ++ .../test/resources/cucumber/DownloadGet.feature | 24 +++++- 18 files changed, 276 insertions(+), 87 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/james-project/blob/11842e0f/mailbox/api/src/main/java/org/apache/james/mailbox/AttachmentManager.java ---------------------------------------------------------------------- diff --git a/mailbox/api/src/main/java/org/apache/james/mailbox/AttachmentManager.java b/mailbox/api/src/main/java/org/apache/james/mailbox/AttachmentManager.java index fd21b54..fb2bc21 100644 --- a/mailbox/api/src/main/java/org/apache/james/mailbox/AttachmentManager.java +++ b/mailbox/api/src/main/java/org/apache/james/mailbox/AttachmentManager.java @@ -37,6 +37,4 @@ public interface AttachmentManager { void storeAttachment(Attachment attachment, MailboxSession mailboxSession) throws MailboxException; void storeAttachmentsForMessage(Collection<Attachment> attachments, MessageId ownerMessageId, MailboxSession mailboxSession) throws MailboxException; - - Collection<MessageId> getRelatedMessageIds(AttachmentId attachmentId, MailboxSession mailboxSession) throws MailboxException; } http://git-wip-us.apache.org/repos/asf/james-project/blob/11842e0f/mailbox/api/src/main/java/org/apache/james/mailbox/MessageIdManager.java ---------------------------------------------------------------------- diff --git a/mailbox/api/src/main/java/org/apache/james/mailbox/MessageIdManager.java b/mailbox/api/src/main/java/org/apache/james/mailbox/MessageIdManager.java index eb35378..309145e 100644 --- a/mailbox/api/src/main/java/org/apache/james/mailbox/MessageIdManager.java +++ b/mailbox/api/src/main/java/org/apache/james/mailbox/MessageIdManager.java @@ -19,7 +19,9 @@ package org.apache.james.mailbox; +import java.util.Collection; import java.util.List; +import java.util.Set; import javax.mail.Flags; @@ -32,6 +34,8 @@ import org.apache.james.mailbox.model.MessageResult.FetchGroup; public interface MessageIdManager { + Set<MessageId> accessibleMessages(Collection<MessageId> messageIds, final MailboxSession mailboxSession) throws MailboxException; + void setFlags(Flags newState, FlagsUpdateMode replace, MessageId messageId, List<MailboxId> mailboxIds, MailboxSession mailboxSession) throws MailboxException; List<MessageResult> getMessages(List<MessageId> messageId, FetchGroup minimal, MailboxSession mailboxSession) throws MailboxException; http://git-wip-us.apache.org/repos/asf/james-project/blob/11842e0f/mailbox/api/src/main/java/org/apache/james/mailbox/model/AttachmentId.java ---------------------------------------------------------------------- diff --git a/mailbox/api/src/main/java/org/apache/james/mailbox/model/AttachmentId.java b/mailbox/api/src/main/java/org/apache/james/mailbox/model/AttachmentId.java index 05c3e62..81a5588 100644 --- a/mailbox/api/src/main/java/org/apache/james/mailbox/model/AttachmentId.java +++ b/mailbox/api/src/main/java/org/apache/james/mailbox/model/AttachmentId.java @@ -40,7 +40,7 @@ public class AttachmentId { private static final String DEFAULT_MIME_TYPE = "application/octet-stream"; public static AttachmentId forPayloadAndType(byte[] payload, String contentType) { - Preconditions.checkArgument(payload != null); + Preconditions.checkNotNull(payload); Preconditions.checkArgument(!Strings.isNullOrEmpty(contentType)); return new AttachmentId(computeRawId(payload, contentType)); @@ -60,8 +60,13 @@ public class AttachmentId { .orElse(DEFAULT_MIME_TYPE); } + public static AttachmentId from(BlobId blobId) { + return new AttachmentId(blobId.asString()); + } + public static AttachmentId from(String id) { - Preconditions.checkArgument(!Strings.isNullOrEmpty(id)); + Preconditions.checkNotNull(id); + Preconditions.checkArgument(!id.isEmpty()); return new AttachmentId(id); } http://git-wip-us.apache.org/repos/asf/james-project/blob/11842e0f/mailbox/api/src/test/java/org/apache/james/mailbox/model/AttachmentIdTest.java ---------------------------------------------------------------------- diff --git a/mailbox/api/src/test/java/org/apache/james/mailbox/model/AttachmentIdTest.java b/mailbox/api/src/test/java/org/apache/james/mailbox/model/AttachmentIdTest.java index 1f56066..148149e 100644 --- a/mailbox/api/src/test/java/org/apache/james/mailbox/model/AttachmentIdTest.java +++ b/mailbox/api/src/test/java/org/apache/james/mailbox/model/AttachmentIdTest.java @@ -20,6 +20,7 @@ package org.apache.james.mailbox.model; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import java.util.UUID; @@ -47,40 +48,54 @@ public class AttachmentIdTest { AttachmentId attachmentId2 = AttachmentId.forPayloadAndType("payload".getBytes(), "text/html; charset=UTF-16"); assertThat(attachmentId.getId()).isEqualTo(attachmentId2.getId()); } - - @Test(expected = IllegalArgumentException.class) + + @Test public void forPayloadAndTypeShouldThrowWhenPayloadIsNull() { - AttachmentId.forPayloadAndType(null, null); + assertThatThrownBy(() -> AttachmentId.forPayloadAndType(null, "text/plain")).isInstanceOf(NullPointerException.class); } - @Test(expected = IllegalArgumentException.class) + @Test public void forPayloadAndTypeShouldThrowWhenTypeIsNull() { - AttachmentId.forPayloadAndType("payload".getBytes(), null); + assertThatThrownBy(() -> AttachmentId.forPayloadAndType("payload".getBytes(), null)).isInstanceOf(IllegalArgumentException.class); } - @Test(expected = IllegalArgumentException.class) + @Test public void forPayloadAndTypeShouldThrowWhenTypeIsEmpty() { - AttachmentId.forPayloadAndType("payload".getBytes(), ""); + assertThatThrownBy(() -> AttachmentId.forPayloadAndType("payload".getBytes(), "")).isInstanceOf(IllegalArgumentException.class); } - @Test(expected = IllegalArgumentException.class) + @Test public void fromShouldThrowWhenIdIsNull() { - AttachmentId.from(null); + String value = null; + assertThatThrownBy(() -> AttachmentId.from(value)).isInstanceOf(NullPointerException.class); + } + + @Test + public void fromShouldThrowWhenBlobIdIsNull() { + BlobId value = null; + assertThatThrownBy(() -> AttachmentId.from(value)).isInstanceOf(NullPointerException.class); } - @Test(expected = IllegalArgumentException.class) + @Test public void fromShouldThrowWhenIdIsEmpty() { - AttachmentId.from(""); + assertThatThrownBy(() -> AttachmentId.from("")).isInstanceOf(IllegalArgumentException.class); } @Test - public void fromShouldWork() { + public void fromStringShouldWork() { String expectedId = "f07e5a815613c5abeddc4b682247a4c42d8a95df"; AttachmentId attachmentId = AttachmentId.from(expectedId); assertThat(attachmentId.getId()).isEqualTo(expectedId); } @Test + public void fromBlobIdShouldWork() { + String expectedId = "f07e5a815613c5abeddc4b682247a4c42d8a95df"; + AttachmentId attachmentId = AttachmentId.from(BlobId.fromString(expectedId)); + assertThat(attachmentId.getId()).isEqualTo(expectedId); + } + + @Test public void asUUIDShouldReturnAValidUUID() { AttachmentId attachmentId = AttachmentId.from("magic"); http://git-wip-us.apache.org/repos/asf/james-project/blob/11842e0f/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/CassandraMailboxSessionMapperFactory.java ---------------------------------------------------------------------- diff --git a/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/CassandraMailboxSessionMapperFactory.java b/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/CassandraMailboxSessionMapperFactory.java index 9d4e059..afffe6d 100644 --- a/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/CassandraMailboxSessionMapperFactory.java +++ b/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/CassandraMailboxSessionMapperFactory.java @@ -183,7 +183,7 @@ public class CassandraMailboxSessionMapperFactory extends MailboxSessionMapperFa } @Override - public AttachmentMapper getAttachmentMapper(MailboxSession session) throws MailboxException { + public AttachmentMapper getAttachmentMapper(MailboxSession session) { AttachmentMapper mapper = (AttachmentMapper) session.getAttributes().get(ATTACHMENTMAPPER); if (mapper == null) { mapper = createAttachmentMapper(session); http://git-wip-us.apache.org/repos/asf/james-project/blob/11842e0f/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageIdMapper.java ---------------------------------------------------------------------- diff --git a/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageIdMapper.java b/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageIdMapper.java index ff9fb69..b5bd133 100644 --- a/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageIdMapper.java +++ b/mailbox/cassandra/src/main/java/org/apache/james/mailbox/cassandra/mail/CassandraMessageIdMapper.java @@ -18,6 +18,7 @@ ****************************************************************/ package org.apache.james.mailbox.cassandra.mail; +import java.util.Collection; import java.util.Comparator; import java.util.List; import java.util.Map; @@ -88,12 +89,12 @@ public class CassandraMessageIdMapper implements MessageIdMapper { } @Override - public List<MailboxMessage> find(List<MessageId> messageIds, FetchType fetchType) { + public List<MailboxMessage> find(Collection<MessageId> messageIds, FetchType fetchType) { return findAsStream(messageIds, fetchType) .collect(Guavate.toImmutableList()); } - private Stream<SimpleMailboxMessage> findAsStream(List<MessageId> messageIds, FetchType fetchType) { + private Stream<SimpleMailboxMessage> findAsStream(Collection<MessageId> messageIds, FetchType fetchType) { return FluentFutureStream.ofNestedStreams( messageIds.stream() .map(messageId -> imapUidDAO.retrieve((CassandraMessageId) messageId, Optional.empty()))) http://git-wip-us.apache.org/repos/asf/james-project/blob/11842e0f/mailbox/memory/src/main/java/org/apache/james/mailbox/inmemory/InMemoryMailboxSessionMapperFactory.java ---------------------------------------------------------------------- diff --git a/mailbox/memory/src/main/java/org/apache/james/mailbox/inmemory/InMemoryMailboxSessionMapperFactory.java b/mailbox/memory/src/main/java/org/apache/james/mailbox/inmemory/InMemoryMailboxSessionMapperFactory.java index adae134..6a4709c 100644 --- a/mailbox/memory/src/main/java/org/apache/james/mailbox/inmemory/InMemoryMailboxSessionMapperFactory.java +++ b/mailbox/memory/src/main/java/org/apache/james/mailbox/inmemory/InMemoryMailboxSessionMapperFactory.java @@ -80,7 +80,7 @@ public class InMemoryMailboxSessionMapperFactory extends MailboxSessionMapperFac } @Override - public AttachmentMapper createAttachmentMapper(MailboxSession session) throws MailboxException { + public AttachmentMapper createAttachmentMapper(MailboxSession session) { return attachmentMapper; } @@ -107,7 +107,7 @@ public class InMemoryMailboxSessionMapperFactory extends MailboxSessionMapperFac } @Override - public AttachmentMapper getAttachmentMapper(MailboxSession session) throws MailboxException { + public AttachmentMapper getAttachmentMapper(MailboxSession session) { return attachmentMapper; } http://git-wip-us.apache.org/repos/asf/james-project/blob/11842e0f/mailbox/memory/src/main/java/org/apache/james/mailbox/inmemory/InMemoryMessageIdManager.java ---------------------------------------------------------------------- diff --git a/mailbox/memory/src/main/java/org/apache/james/mailbox/inmemory/InMemoryMessageIdManager.java b/mailbox/memory/src/main/java/org/apache/james/mailbox/inmemory/InMemoryMessageIdManager.java index 6d63a64..2b0256c 100644 --- a/mailbox/memory/src/main/java/org/apache/james/mailbox/inmemory/InMemoryMessageIdManager.java +++ b/mailbox/memory/src/main/java/org/apache/james/mailbox/inmemory/InMemoryMessageIdManager.java @@ -19,10 +19,14 @@ package org.apache.james.mailbox.inmemory; +import java.util.Collection; import java.util.HashSet; import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.function.Predicate; +import java.util.stream.Stream; + import javax.inject.Inject; import javax.mail.Flags; @@ -42,7 +46,9 @@ import org.apache.james.mailbox.model.MessageId; import org.apache.james.mailbox.model.MessageRange; import org.apache.james.mailbox.model.MessageResult; import org.apache.james.mailbox.model.MessageResult.FetchGroup; +import org.apache.james.util.OptionalUtils; +import com.github.fge.lambdas.Throwing; import com.github.steveash.guavate.Guavate; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -58,7 +64,7 @@ public class InMemoryMessageIdManager implements MessageIdManager { public InMemoryMessageIdManager(MailboxManager mailboxManager) { this.mailboxManager = mailboxManager; } - + @Override public void setFlags(Flags newState, FlagsUpdateMode flagsUpdateMode, MessageId messageId, List<MailboxId> mailboxIds, MailboxSession mailboxSession) throws MailboxException { for (MailboxId mailboxId: mailboxIds) { @@ -71,13 +77,21 @@ public class InMemoryMessageIdManager implements MessageIdManager { } @Override + public Set<MessageId> accessibleMessages(Collection<MessageId> messageIds, MailboxSession mailboxSession) throws MailboxException { + return getUsersMailboxIds(mailboxSession) + .stream() + .flatMap(Throwing.function(mailboxId -> retrieveMailboxMessages(mailboxId, messageIds, FetchGroupImpl.MINIMAL, mailboxSession))) + .map(MessageResult::getMessageId) + .collect(Guavate.toImmutableSet()); + } + + + @Override public List<MessageResult> getMessages(List<MessageId> messages, FetchGroup fetchGroup, final MailboxSession mailboxSession) throws MailboxException { - ImmutableList.Builder<MessageResult> builder = ImmutableList.builder(); - List<MailboxId> userMailboxIds = getUsersMailboxIds(mailboxSession); - for (MailboxId mailboxId: userMailboxIds) { - builder.addAll(retrieveMailboxMessages(mailboxId, messages, fetchGroup, mailboxSession)); - } - return builder.build(); + return getUsersMailboxIds(mailboxSession) + .stream() + .flatMap(Throwing.function(mailboxId -> retrieveMailboxMessages(mailboxId, messages, FetchGroupImpl.MINIMAL, mailboxSession))) + .collect(Guavate.toImmutableList()); } @Override @@ -136,15 +150,10 @@ public class InMemoryMessageIdManager implements MessageIdManager { .build(); } - private List<MessageResult> retrieveMailboxMessages(MailboxId mailboxId, List<MessageId> messages, FetchGroup fetchGroup, MailboxSession mailboxSession) throws MailboxException { - ImmutableList.Builder<MessageResult> builder = ImmutableList.builder(); - for (MessageId messageId: messages) { - Optional<MessageResult> maybeMessage = findMessageWithId(mailboxId, messageId, fetchGroup, mailboxSession); - if (maybeMessage.isPresent()) { - builder.add(maybeMessage.get()); - } - } - return builder.build(); + private Stream<MessageResult> retrieveMailboxMessages(MailboxId mailboxId, Collection<MessageId> messages, FetchGroup fetchGroup, MailboxSession mailboxSession) { + return messages.stream() + .map(Throwing.function(messageId -> findMessageWithId(mailboxId, messageId, fetchGroup, mailboxSession))) + .flatMap(OptionalUtils::toStream); } private void filterOnMailboxSession(List<MailboxId> mailboxIds, MailboxSession mailboxSession) throws MailboxNotFoundException { http://git-wip-us.apache.org/repos/asf/james-project/blob/11842e0f/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreAttachmentManager.java ---------------------------------------------------------------------- diff --git a/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreAttachmentManager.java b/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreAttachmentManager.java index 1ee0c90..87de834 100644 --- a/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreAttachmentManager.java +++ b/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreAttachmentManager.java @@ -26,53 +26,71 @@ import javax.inject.Inject; import org.apache.james.mailbox.AttachmentManager; import org.apache.james.mailbox.MailboxSession; +import org.apache.james.mailbox.MessageIdManager; import org.apache.james.mailbox.exception.AttachmentNotFoundException; import org.apache.james.mailbox.exception.MailboxException; import org.apache.james.mailbox.model.Attachment; import org.apache.james.mailbox.model.AttachmentId; import org.apache.james.mailbox.model.MessageId; -import org.apache.james.mailbox.store.mail.AttachmentMapper; import org.apache.james.mailbox.store.mail.AttachmentMapperFactory; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import com.github.steveash.guavate.Guavate; +import com.google.common.base.Throwables; public class StoreAttachmentManager implements AttachmentManager { + private static final Logger LOGGER = LoggerFactory.getLogger(StoreAttachmentManager.class); private final AttachmentMapperFactory attachmentMapperFactory; + private final MessageIdManager messageIdManager; @Inject - public StoreAttachmentManager(AttachmentMapperFactory attachmentMapperFactory) { + public StoreAttachmentManager(AttachmentMapperFactory attachmentMapperFactory, MessageIdManager messageIdManager) { this.attachmentMapperFactory = attachmentMapperFactory; - } - - protected AttachmentMapperFactory getAttachmentMapperFactory() { - return attachmentMapperFactory; - } - - protected AttachmentMapper getAttachmentMapper(MailboxSession mailboxSession) throws MailboxException { - return attachmentMapperFactory.getAttachmentMapper(mailboxSession); + this.messageIdManager = messageIdManager; } @Override public Attachment getAttachment(AttachmentId attachmentId, MailboxSession mailboxSession) throws MailboxException, AttachmentNotFoundException { - return getAttachmentMapper(mailboxSession).getAttachment(attachmentId); + if (!userHasAccessToAttachment(attachmentId, mailboxSession)) { + throw new AttachmentNotFoundException(attachmentId.getId()); + } + return attachmentMapperFactory.getAttachmentMapper(mailboxSession).getAttachment(attachmentId); } @Override public List<Attachment> getAttachments(List<AttachmentId> attachmentIds, MailboxSession mailboxSession) throws MailboxException { - return getAttachmentMapper(mailboxSession).getAttachments(attachmentIds); + List<AttachmentId> accessibleAttachmentIds = attachmentIds.stream() + .filter(attachmentId -> userHasAccessToAttachment(attachmentId, mailboxSession)) + .collect(Guavate.toImmutableList()); + + return attachmentMapperFactory.getAttachmentMapper(mailboxSession).getAttachments(accessibleAttachmentIds); } @Override public void storeAttachment(Attachment attachment, MailboxSession mailboxSession) throws MailboxException { - getAttachmentMapper(mailboxSession).storeAttachment(attachment); + attachmentMapperFactory.getAttachmentMapper(mailboxSession).storeAttachment(attachment); } @Override public void storeAttachmentsForMessage(Collection<Attachment> attachments, MessageId ownerMessageId, MailboxSession mailboxSession) throws MailboxException { - getAttachmentMapper(mailboxSession).storeAttachmentsForMessage(attachments, ownerMessageId); + attachmentMapperFactory.getAttachmentMapper(mailboxSession).storeAttachmentsForMessage(attachments, ownerMessageId); } - @Override - public Collection<MessageId> getRelatedMessageIds(AttachmentId attachmentId, MailboxSession mailboxSession) throws MailboxException { - return getAttachmentMapper(mailboxSession).getRelatedMessageIds(attachmentId); + private boolean userHasAccessToAttachment(AttachmentId attachmentId, MailboxSession mailboxSession) { + try { + return !messageIdManager + .accessibleMessages(getRelatedMessageIds(attachmentId, mailboxSession), mailboxSession) + .isEmpty(); + } catch (MailboxException e) { + LOGGER.warn("Error while checking attachment related accessible message ids", e); + throw Throwables.propagate(e); + } } + + private Collection<MessageId> getRelatedMessageIds(AttachmentId attachmentId, MailboxSession mailboxSession) throws MailboxException { + return attachmentMapperFactory.getAttachmentMapper(mailboxSession).getRelatedMessageIds(attachmentId); + } + } http://git-wip-us.apache.org/repos/asf/james-project/blob/11842e0f/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreBlobManager.java ---------------------------------------------------------------------- diff --git a/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreBlobManager.java b/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreBlobManager.java index c70d479..ccb4f98 100644 --- a/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreBlobManager.java +++ b/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreBlobManager.java @@ -19,7 +19,7 @@ package org.apache.james.mailbox.store; -import java.io.IOException; +import java.io.InputStream; import java.util.Optional; import javax.inject.Inject; @@ -32,14 +32,13 @@ import org.apache.james.mailbox.MessageIdManager; import org.apache.james.mailbox.exception.AttachmentNotFoundException; import org.apache.james.mailbox.exception.BlobNotFoundException; import org.apache.james.mailbox.exception.MailboxException; -import org.apache.james.mailbox.model.Attachment; import org.apache.james.mailbox.model.AttachmentId; import org.apache.james.mailbox.model.Blob; import org.apache.james.mailbox.model.BlobId; import org.apache.james.mailbox.model.FetchGroupImpl; import org.apache.james.mailbox.model.MessageId; -import org.apache.james.mailbox.model.MessageResult; +import com.github.fge.lambdas.Throwing; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; @@ -50,7 +49,8 @@ public class StoreBlobManager implements BlobManager { private final MessageId.Factory messageIdFactory; @Inject - public StoreBlobManager(AttachmentManager attachmentManager, MessageIdManager messageIdManager, MessageId.Factory messageIdFactory) { + public StoreBlobManager(AttachmentManager attachmentManager, MessageIdManager messageIdManager, + MessageId.Factory messageIdFactory) { this.attachmentManager = attachmentManager; this.messageIdManager = messageIdManager; this.messageIdFactory = messageIdFactory; @@ -64,16 +64,14 @@ public class StoreBlobManager implements BlobManager { @Override public Blob retrieve(BlobId blobId, MailboxSession mailboxSession) throws MailboxException, BlobNotFoundException { return getBlobFromAttachment(blobId, mailboxSession) - .orElseGet(() -> getBlobFromMessage(blobId, mailboxSession) + .orElseGet(() -> getBlobFromMessage(blobId, mailboxSession) .orElseThrow(() -> new BlobNotFoundException(blobId))); } private Optional<Blob> getBlobFromAttachment(BlobId blobId, MailboxSession mailboxSession) throws MailboxException { try { - Attachment attachment = attachmentManager.getAttachment( - AttachmentId.from(blobId.asString()), - mailboxSession); - return Optional.of(attachment.toBlob()); + AttachmentId attachmentId = AttachmentId.from(blobId); + return Optional.of(attachmentManager.getAttachment(attachmentId, mailboxSession).toBlob()); } catch (AttachmentNotFoundException e) { return Optional.empty(); } @@ -81,37 +79,32 @@ public class StoreBlobManager implements BlobManager { private Optional<Blob> getBlobFromMessage(BlobId blobId, MailboxSession mailboxSession) { return retrieveMessageId(blobId) - .flatMap(messageId -> fromMessageId(blobId, mailboxSession, messageId)); + .flatMap(messageId -> loadMessageAsBlob(messageId, mailboxSession)) + .map(Throwing.function( + blob -> Blob.builder() + .id(blobId) + .contentType(MESSAGE_RFC822_CONTENT_TYPE) + .payload(IOUtils.toByteArray(blob)) + .build())); } private Optional<MessageId> retrieveMessageId(BlobId blobId) { try { return Optional.of(messageIdFactory.fromString(blobId.asString())); - } catch (Exception e) { + } catch (IllegalArgumentException e) { return Optional.empty(); } } - private Optional<Blob> fromMessageId(BlobId blobId, MailboxSession mailboxSession, MessageId messageId) { + private Optional<InputStream> loadMessageAsBlob(MessageId messageId, MailboxSession mailboxSession) { try { return messageIdManager.getMessages(ImmutableList.of(messageId), FetchGroupImpl.FULL_CONTENT, mailboxSession) .stream() - .findFirst() - .map(messageResult -> toBlob(blobId, messageResult)); + .map(Throwing.function(message -> message.getFullContent().getInputStream())) + .findFirst(); } catch (MailboxException e) { throw Throwables.propagate(e); } } - private Blob toBlob(BlobId blobId, MessageResult messageResult) { - try { - return Blob.builder() - .id(blobId) - .contentType(MESSAGE_RFC822_CONTENT_TYPE) - .payload(IOUtils.toByteArray(messageResult.getFullContent().getInputStream())) - .build(); - } catch (IOException | MailboxException e) { - throw Throwables.propagate(e); - } - } } http://git-wip-us.apache.org/repos/asf/james-project/blob/11842e0f/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMessageIdManager.java ---------------------------------------------------------------------- diff --git a/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMessageIdManager.java b/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMessageIdManager.java index 11759cb..f167a9e 100644 --- a/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMessageIdManager.java +++ b/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMessageIdManager.java @@ -26,6 +26,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.function.Function; import java.util.function.Predicate; import javax.inject.Inject; @@ -99,6 +100,17 @@ public class StoreMessageIdManager implements MessageIdManager { } @Override + public Set<MessageId> accessibleMessages(Collection<MessageId> messageIds, final MailboxSession mailboxSession) throws MailboxException { + MessageIdMapper messageIdMapper = mailboxSessionMapperFactory.getMessageIdMapper(mailboxSession); + final MailboxMapper mailboxMapper = mailboxSessionMapperFactory.getMailboxMapper(mailboxSession); + List<MailboxMessage> messageList = messageIdMapper.find(messageIds, MessageMapper.FetchType.Metadata); + return messageList.stream() + .filter(message -> mailboxBelongsToUser(mailboxSession, mailboxMapper).test(message.getMailboxId())) + .map(message -> message.getComposedMessageIdWithMetaData().getComposedMessageId().getMessageId()) + .collect(Guavate.toImmutableSet()); + } + + @Override public List<MessageResult> getMessages(List<MessageId> messageIds, final MessageResult.FetchGroup fetchGroup, final MailboxSession mailboxSession) throws MailboxException { try { MessageIdMapper messageIdMapper = mailboxSessionMapperFactory.getMessageIdMapper(mailboxSession); http://git-wip-us.apache.org/repos/asf/james-project/blob/11842e0f/mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/AttachmentMapperFactory.java ---------------------------------------------------------------------- diff --git a/mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/AttachmentMapperFactory.java b/mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/AttachmentMapperFactory.java index e3d768d..7747067 100644 --- a/mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/AttachmentMapperFactory.java +++ b/mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/AttachmentMapperFactory.java @@ -19,11 +19,10 @@ package org.apache.james.mailbox.store.mail; import org.apache.james.mailbox.MailboxSession; -import org.apache.james.mailbox.exception.MailboxException; public interface AttachmentMapperFactory { - AttachmentMapper createAttachmentMapper(MailboxSession session) throws MailboxException; + AttachmentMapper createAttachmentMapper(MailboxSession session); - AttachmentMapper getAttachmentMapper(MailboxSession session) throws MailboxException; + AttachmentMapper getAttachmentMapper(MailboxSession session); } http://git-wip-us.apache.org/repos/asf/james-project/blob/11842e0f/mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/MessageIdMapper.java ---------------------------------------------------------------------- diff --git a/mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/MessageIdMapper.java b/mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/MessageIdMapper.java index e517e99..1e9a546 100644 --- a/mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/MessageIdMapper.java +++ b/mailbox/store/src/main/java/org/apache/james/mailbox/store/mail/MessageIdMapper.java @@ -18,6 +18,7 @@ ****************************************************************/ package org.apache.james.mailbox.store.mail; +import java.util.Collection; import java.util.List; import java.util.Map; @@ -34,7 +35,7 @@ import org.apache.james.mailbox.store.mail.model.MailboxMessage; public interface MessageIdMapper { - List<MailboxMessage> find(List<MessageId> messageIds, FetchType fetchType); + List<MailboxMessage> find(Collection<MessageId> messageIds, FetchType fetchType); List<MailboxId> findMailboxes(MessageId messageId); http://git-wip-us.apache.org/repos/asf/james-project/blob/11842e0f/mailbox/store/src/test/java/org/apache/james/mailbox/store/AbstractMessageIdManagerStorageTest.java ---------------------------------------------------------------------- diff --git a/mailbox/store/src/test/java/org/apache/james/mailbox/store/AbstractMessageIdManagerStorageTest.java b/mailbox/store/src/test/java/org/apache/james/mailbox/store/AbstractMessageIdManagerStorageTest.java index 1d964cb..757b276 100644 --- a/mailbox/store/src/test/java/org/apache/james/mailbox/store/AbstractMessageIdManagerStorageTest.java +++ b/mailbox/store/src/test/java/org/apache/james/mailbox/store/AbstractMessageIdManagerStorageTest.java @@ -500,6 +500,34 @@ public abstract class AbstractMessageIdManagerStorageTest { assertThat(messageResults.get(0).getMailboxId()).isEqualTo(mailbox2.getMailboxId()); } + @Test + public void accessibleMessagesShouldReturnMessageIdsThatBelongsToTheUser() throws Exception { + MessageId messageId = testingData.persist(mailbox1.getMailboxId(), messageUid1, FLAGS, session); + + assertThat(messageIdManager.accessibleMessages(ImmutableList.of(messageId), session)) + .containsExactly(messageId); + } + + @Test + public void accessibleMessagesShouldReturnEmptyWhenSuppliedMessageIdsAreEmpty() throws Exception { + assertThat(messageIdManager.accessibleMessages(ImmutableList.of(), session)) + .isEmpty(); + } + + @Test + public void accessibleMessagesShouldFilterOutMessageIdsWhenNotExisting() throws Exception { + assertThat(messageIdManager.accessibleMessages(ImmutableList.of(testingData.createNotUsedMessageId()), session)) + .isEmpty(); + } + + @Test + public void accessibleMessagesShouldFilterOutMessageIdsWhenNotBelongingToTheUser() throws Exception { + MessageId messageId = testingData.persist(mailbox1.getMailboxId(), messageUid1, FLAGS, session); + + assertThat(messageIdManager.accessibleMessages(ImmutableList.of(messageId), otherSession)) + .isEmpty(); + } + private Predicate<MessageResult> inMailbox(final MailboxId mailboxId) { return messageResult -> messageResult.getMailboxId().equals(mailboxId); } http://git-wip-us.apache.org/repos/asf/james-project/blob/11842e0f/mailbox/store/src/test/java/org/apache/james/mailbox/store/StoreAttachmentManagerTest.java ---------------------------------------------------------------------- diff --git a/mailbox/store/src/test/java/org/apache/james/mailbox/store/StoreAttachmentManagerTest.java b/mailbox/store/src/test/java/org/apache/james/mailbox/store/StoreAttachmentManagerTest.java new file mode 100644 index 0000000..f205808 --- /dev/null +++ b/mailbox/store/src/test/java/org/apache/james/mailbox/store/StoreAttachmentManagerTest.java @@ -0,0 +1,80 @@ +/**************************************************************** + * 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.store; + +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Matchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.nio.charset.StandardCharsets; + +import org.apache.james.mailbox.MailboxSession; +import org.apache.james.mailbox.MessageIdManager; +import org.apache.james.mailbox.exception.AttachmentNotFoundException; +import org.apache.james.mailbox.model.Attachment; +import org.apache.james.mailbox.model.AttachmentId; +import org.apache.james.mailbox.model.MessageId; +import org.apache.james.mailbox.model.TestMessageId; +import org.apache.james.mailbox.store.mail.AttachmentMapper; +import org.apache.james.mailbox.store.mail.AttachmentMapperFactory; +import org.junit.Before; +import org.junit.Test; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; + +public class StoreAttachmentManagerTest { + private static final TestMessageId MESSAGE_ID = TestMessageId.of(1L); + private static final ImmutableList<MessageId> MESSAGE_IDS = ImmutableList.of(MESSAGE_ID); + private static final AttachmentId ATTACHMENT_ID = AttachmentId.from("1"); + private static final Attachment ATTACHMENT = Attachment.builder() + .attachmentId(ATTACHMENT_ID) + .type("type") + .bytes("Any".getBytes(StandardCharsets.UTF_8)) + .build(); + + private StoreAttachmentManager testee; + private AttachmentMapper attachmentMapper; + private MessageIdManager messageIdManager; + + @Before + public void setup() throws Exception { + attachmentMapper = mock(AttachmentMapper.class); + AttachmentMapperFactory attachmentMapperFactory = mock(AttachmentMapperFactory.class); + when(attachmentMapperFactory.getAttachmentMapper(any(MailboxSession.class))) + .thenReturn(attachmentMapper); + messageIdManager = mock(MessageIdManager.class); + + testee = new StoreAttachmentManager(attachmentMapperFactory, messageIdManager); + } + + @Test + public void getAttachmentShouldThrowWhenAttachmentDoesNotBelongToUser() throws Exception { + MailboxSession mailboxSession = mock(MailboxSession.class); + when(attachmentMapper.getAttachment(ATTACHMENT_ID)).thenReturn(ATTACHMENT); + when(attachmentMapper.getRelatedMessageIds(ATTACHMENT_ID)).thenReturn(MESSAGE_IDS); + when(messageIdManager.accessibleMessages(MESSAGE_IDS, mailboxSession)).thenReturn(ImmutableSet.of()); + + assertThatThrownBy(() -> testee.getAttachment(ATTACHMENT_ID, mailboxSession)) + .isInstanceOf(AttachmentNotFoundException.class); + } + +} \ No newline at end of file http://git-wip-us.apache.org/repos/asf/james-project/blob/11842e0f/mailbox/store/src/test/java/org/apache/james/mailbox/store/TestMailboxSessionMapperFactory.java ---------------------------------------------------------------------- diff --git a/mailbox/store/src/test/java/org/apache/james/mailbox/store/TestMailboxSessionMapperFactory.java b/mailbox/store/src/test/java/org/apache/james/mailbox/store/TestMailboxSessionMapperFactory.java index a2d7b03..5a3937c 100644 --- a/mailbox/store/src/test/java/org/apache/james/mailbox/store/TestMailboxSessionMapperFactory.java +++ b/mailbox/store/src/test/java/org/apache/james/mailbox/store/TestMailboxSessionMapperFactory.java @@ -25,6 +25,7 @@ import static org.mockito.Mockito.when; import java.util.AbstractMap; import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.function.Function; @@ -160,7 +161,7 @@ public class TestMailboxSessionMapperFactory extends MailboxSessionMapperFactory messageIdMapper = new MessageIdMapper() { @Override - public List<MailboxMessage> find(final List<MessageId> messageIds, MessageMapper.FetchType fetchType) { + public List<MailboxMessage> find(final Collection<MessageId> messageIds, MessageMapper.FetchType fetchType) { return messages.stream() .filter(withMessageIdOneOf(messageIds)) .collect(Guavate.toImmutableList()); @@ -285,7 +286,7 @@ public class TestMailboxSessionMapperFactory extends MailboxSessionMapperFactory messages.clear(); } - private Predicate<MailboxMessage> withMessageIdOneOf(final List<MessageId> messageIds) { + private Predicate<MailboxMessage> withMessageIdOneOf(final Collection<MessageId> messageIds) { return mailboxMessage -> messageIds.contains(mailboxMessage.getMessageId()); } http://git-wip-us.apache.org/repos/asf/james-project/blob/11842e0f/server/protocols/jmap-integration-testing/jmap-integration-testing-common/src/test/java/org/apache/james/jmap/methods/integration/cucumber/DownloadStepdefs.java ---------------------------------------------------------------------- diff --git a/server/protocols/jmap-integration-testing/jmap-integration-testing-common/src/test/java/org/apache/james/jmap/methods/integration/cucumber/DownloadStepdefs.java b/server/protocols/jmap-integration-testing/jmap-integration-testing-common/src/test/java/org/apache/james/jmap/methods/integration/cucumber/DownloadStepdefs.java index ac1be1f..c872e53 100644 --- a/server/protocols/jmap-integration-testing/jmap-integration-testing-common/src/test/java/org/apache/james/jmap/methods/integration/cucumber/DownloadStepdefs.java +++ b/server/protocols/jmap-integration-testing/jmap-integration-testing-common/src/test/java/org/apache/james/jmap/methods/integration/cucumber/DownloadStepdefs.java @@ -340,6 +340,11 @@ public class DownloadStepdefs { .returnResponse(); } + @When("^\"([^\"]*)\" delete mailbox \"([^\"]*)\"$") + public void deleteMailboxButNotAttachment(String username, String mailboxName) throws Exception { + mainStepdefs.jmapServer.getProbe(MailboxProbeImpl.class).deleteMailbox(MailboxConstants.USER_NAMESPACE, username, mailboxName); + } + @Then("^the user should be authorized$") public void httpStatusDifferentFromUnauthorized() throws IOException { assertThat(response.getStatusLine().getStatusCode()).isIn(200, 404); http://git-wip-us.apache.org/repos/asf/james-project/blob/11842e0f/server/protocols/jmap-integration-testing/jmap-integration-testing-common/src/test/resources/cucumber/DownloadGet.feature ---------------------------------------------------------------------- diff --git a/server/protocols/jmap-integration-testing/jmap-integration-testing-common/src/test/resources/cucumber/DownloadGet.feature b/server/protocols/jmap-integration-testing/jmap-integration-testing-common/src/test/resources/cucumber/DownloadGet.feature index b450533..2487487 100644 --- a/server/protocols/jmap-integration-testing/jmap-integration-testing-common/src/test/resources/cucumber/DownloadGet.feature +++ b/server/protocols/jmap-integration-testing/jmap-integration-testing-common/src/test/resources/cucumber/DownloadGet.feature @@ -1,6 +1,6 @@ Feature: Download GET As a James user - I want to retrieve my attachments + I want to retrieve my blobs (attachments and messages) Background: Given a domain named "domain.tld" @@ -41,4 +41,24 @@ Feature: Download GET And the user ask for messages "m1" When "usern...@domain.tld" downloads the message by its blobId Then the user should receive that blob - And the blob size is 36 \ No newline at end of file + And the blob size is 36 + + Scenario: Deleted message should revoke attachment blob download rights + Given "usern...@domain.tld" mailbox "INBOX" contains a message "1" with an attachment "2" + And "usern...@domain.tld" delete mailbox "INBOX" + When "usern...@domain.tld" downloads "2" + Then the user should receive a not found response + + Scenario: User cannot download attachment of another user + Given "usern...@domain.tld" mailbox "INBOX" contains a message "1" with an attachment "2" + And a connected user "userna...@domain.tld" + And "userna...@domain.tld" has a mailbox "INBOX" + When "userna...@domain.tld" downloads "2" + Then the user should receive a not found response + + Scenario: User cannot download message blob of another user + Given "usern...@domain.tld" mailbox "INBOX" contains a message "1" with an attachment "2" + And a connected user "userna...@domain.tld" + And "userna...@domain.tld" has a mailbox "INBOX" + When "userna...@domain.tld" downloads "1" + Then the user should receive a not found response \ No newline at end of file --------------------------------------------------------------------- To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org For additional commands, e-mail: server-dev-h...@james.apache.org
