james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From adup...@apache.org
Subject [3/5] james-project git commit: JAMES-2145 Attachment security, user cannot download attachment of another user
Date Wed, 20 Sep 2017 20:31:58 GMT
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 <qnguyen@linagora.com>
Authored: Mon Sep 18 18:03:15 2017 +0700
Committer: Antoine Duprat <aduprat@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 "username@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 "username@domain.tld" mailbox "INBOX" contains a message "1" with an attachment
"2"
+    And "username@domain.tld" delete mailbox "INBOX"
+    When "username@domain.tld" downloads "2"
+    Then the user should receive a not found response
+
+  Scenario: User cannot download attachment of another user
+    Given "username@domain.tld" mailbox "INBOX" contains a message "1" with an attachment
"2"
+    And a connected user "username1@domain.tld"
+    And "username1@domain.tld" has a mailbox "INBOX"
+    When "username1@domain.tld" downloads "2"
+    Then the user should receive a not found response
+
+  Scenario: User cannot download message blob of another user
+    Given "username@domain.tld" mailbox "INBOX" contains a message "1" with an attachment
"2"
+    And a connected user "username1@domain.tld"
+    And "username1@domain.tld" has a mailbox "INBOX"
+    When "username1@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-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Mime
View raw message