This is an automated email from the ASF dual-hosted git repository. rcordier pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/james-project.git
commit bb4029d48f7888a90e02b9ea30c7cd0c7554e4d9 Author: LanKhuat <dlkh...@linagora.com> AuthorDate: Tue Aug 18 11:54:58 2020 +0700 JAMES-3361 JMAP Draft: sharee should not be able to modify mailbox rights --- .../apache/james/mailbox/MailboxManagerTest.java | 95 ++++++++++++++++++++++ .../james/mailbox/store/StoreRightManager.java | 38 ++++++--- .../cucumber/SetMailboxesMethodStepdefs.java | 46 ++++++++--- .../sharing/MailboxCreationAndSharing.feature | 4 + 4 files changed, 161 insertions(+), 22 deletions(-) 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 78d528e..04a4a68 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 @@ -2722,9 +2722,15 @@ public abstract class MailboxManagerTest<T extends MailboxManager> { @Nested class RightTests { + + private MailboxSession session2; + @BeforeEach void setUp() { + assumeTrue(mailboxManager.hasCapability(MailboxCapabilities.ACL)); + session = mailboxManager.createSystemSession(USER_1); + session2 = mailboxManager.createSystemSession(USER_2); } @Test @@ -2796,5 +2802,94 @@ public abstract class MailboxManagerTest<T extends MailboxManager> { assertThatThrownBy(() -> mailboxManager.setRights(id, MailboxACL.EMPTY, session)) .isInstanceOf(MailboxNotFoundException.class); } + + @Test + void setRightsByIdShouldThrowWhenNotOwner() throws Exception { + MailboxId id = mailboxManager.createMailbox(MailboxPath.forUser(USER_2, "mailbox"), session2).get(); + mailboxManager.setRights(id, MailboxACL.EMPTY.apply(MailboxACL.command() + .key(MailboxACL.EntryKey.createUserEntryKey(USER_1)) + .rights(new MailboxACL.Rfc4314Rights(MailboxACL.Right.Lookup)) + .asAddition()), session2); + + assertThatThrownBy(() -> mailboxManager.setRights(id, MailboxACL.EMPTY.apply( + MailboxACL.command() + .key(MailboxACL.EntryKey.createUserEntryKey(USER_1)) + .rights(MailboxACL.FULL_RIGHTS) + .asAddition()), session)) + .isInstanceOf(InsufficientRightsException.class); + } + + @Test + void setRightsByPathShouldThrowWhenNotOwner() throws Exception { + MailboxPath mailboxPath = MailboxPath.forUser(USER_2, "mailbox"); + mailboxManager.createMailbox(mailboxPath, session2).get(); + mailboxManager.setRights(mailboxPath, MailboxACL.EMPTY.apply(MailboxACL.command() + .key(MailboxACL.EntryKey.createUserEntryKey(USER_1)) + .rights(new MailboxACL.Rfc4314Rights(MailboxACL.Right.Lookup)) + .asAddition()), session2); + + assertThatThrownBy(() -> mailboxManager.setRights(mailboxPath, MailboxACL.EMPTY.apply( + MailboxACL.command() + .key(MailboxACL.EntryKey.createUserEntryKey(USER_1)) + .rights(MailboxACL.FULL_RIGHTS) + .asAddition()), session)) + .isInstanceOf(InsufficientRightsException.class); + } + + @Test + void applyRightsCommandShouldThrowWhenNotOwner() throws Exception { + MailboxPath mailboxPath = MailboxPath.forUser(USER_2, "mailbox"); + mailboxManager.createMailbox(mailboxPath, session2).get(); + mailboxManager.setRights(mailboxPath, MailboxACL.EMPTY.apply(MailboxACL.command() + .key(MailboxACL.EntryKey.createUserEntryKey(USER_1)) + .rights(new MailboxACL.Rfc4314Rights(MailboxACL.Right.Lookup)) + .asAddition()), session2); + + assertThatThrownBy(() -> mailboxManager.applyRightsCommand(mailboxPath, + MailboxACL.command() + .key(MailboxACL.EntryKey.createUserEntryKey(USER_1)) + .rights(MailboxACL.FULL_RIGHTS) + .asAddition(), session)) + .isInstanceOf(InsufficientRightsException.class); + } + + @Test + void setRightsByIdShouldThrowWhenNoRights() throws Exception { + MailboxPath mailboxPath = MailboxPath.forUser(USER_2, "mailbox"); + MailboxId mailboxId = mailboxManager.createMailbox(mailboxPath, session2).get(); + + assertThatThrownBy(() -> mailboxManager.setRights(mailboxId, MailboxACL.EMPTY.apply( + MailboxACL.command() + .key(MailboxACL.EntryKey.createUserEntryKey(USER_1)) + .rights(MailboxACL.FULL_RIGHTS) + .asAddition()), session)) + .isInstanceOf(MailboxNotFoundException.class); + } + + @Test + void setRightsByPathShouldThrowWhenNoRights() throws Exception { + MailboxPath mailboxPath = MailboxPath.forUser(USER_2, "mailbox"); + mailboxManager.createMailbox(mailboxPath, session2).get(); + + assertThatThrownBy(() -> mailboxManager.setRights(mailboxPath, MailboxACL.EMPTY.apply( + MailboxACL.command() + .key(MailboxACL.EntryKey.createUserEntryKey(USER_1)) + .rights(MailboxACL.FULL_RIGHTS) + .asAddition()), session)) + .isInstanceOf(MailboxNotFoundException.class); + } + + @Test + void applyRightsCommandShouldThrowWhenNoRights() throws Exception { + MailboxPath mailboxPath = MailboxPath.forUser(USER_2, "mailbox"); + mailboxManager.createMailbox(mailboxPath, session2).get(); + + assertThatThrownBy(() -> mailboxManager.applyRightsCommand(mailboxPath, + MailboxACL.command() + .key(MailboxACL.EntryKey.createUserEntryKey(USER_1)) + .rights(MailboxACL.FULL_RIGHTS) + .asAddition(), session)) + .isInstanceOf(MailboxNotFoundException.class); + } } } diff --git a/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreRightManager.java b/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreRightManager.java index ddb5e6d..afd7b90 100644 --- a/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreRightManager.java +++ b/mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreRightManager.java @@ -38,6 +38,7 @@ import org.apache.james.mailbox.acl.MailboxACLResolver; import org.apache.james.mailbox.events.EventBus; import org.apache.james.mailbox.events.MailboxIdRegistrationKey; import org.apache.james.mailbox.exception.DifferentDomainException; +import org.apache.james.mailbox.exception.InsufficientRightsException; import org.apache.james.mailbox.exception.MailboxException; import org.apache.james.mailbox.exception.MailboxNotFoundException; import org.apache.james.mailbox.exception.UnsupportedRightException; @@ -148,14 +149,18 @@ public class StoreRightManager implements RightManager { assertSharesBelongsToUserDomain(mailboxPath.getUser(), mailboxACLCommand); MailboxMapper mapper = mailboxSessionMapperFactory.getMailboxMapper(session); block(mapper.findMailboxByPath(mailboxPath) - .flatMap(mailbox -> mapper.updateACL(mailbox, mailboxACLCommand) - .flatMap(aclDiff -> eventBus.dispatch(EventFactory.aclUpdated() - .randomEventId() - .mailboxSession(session) - .mailbox(mailbox) - .aclDiff(aclDiff) - .build(), - new MailboxIdRegistrationKey(mailbox.getMailboxId()))))); + .flatMap(Throwing.<Mailbox, Mono<Void>>function(mailbox -> { + assertHaveAccessTo(mailbox, session); + + return mapper.updateACL(mailbox, mailboxACLCommand) + .flatMap(aclDiff -> eventBus.dispatch(EventFactory.aclUpdated() + .randomEventId() + .mailboxSession(session) + .mailbox(mailbox) + .aclDiff(aclDiff) + .build(), + new MailboxIdRegistrationKey(mailbox.getMailboxId()))); + }).sneakyThrow())); } private void assertSharesBelongsToUserDomain(Username user, ACLCommand mailboxACLCommand) throws DifferentDomainException { @@ -208,10 +213,23 @@ public class StoreRightManager implements RightManager { @Override public void setRights(MailboxPath mailboxPath, MailboxACL mailboxACL, MailboxSession session) throws MailboxException { assertSharesBelongsToUserDomain(mailboxPath.getUser(), mailboxACL.getEntries()); - MailboxMapper mapper = mailboxSessionMapperFactory.getMailboxMapper(session); + MailboxMapper mapper = mailboxSessionMapperFactory.getMailboxMapper(session); block(mapper.findMailboxByPath(mailboxPath) - .flatMap(mailbox -> setRights(mailboxACL, mapper, mailbox, session))); + .flatMap(Throwing.<Mailbox, Mono<Void>>function(mailbox -> { + assertHaveAccessTo(mailbox, session); + return setRights(mailboxACL, mapper, mailbox, session); + }).sneakyThrow())); + } + + private void assertHaveAccessTo(Mailbox mailbox, MailboxSession session) throws InsufficientRightsException, MailboxNotFoundException { + if (!mailbox.generateAssociatedPath().belongsTo(session)) { + if (mailbox.getACL().getEntries().containsKey(EntryKey.createUserEntryKey(session.getUser()))) { + throw new InsufficientRightsException("Setting ACL is only permitted to the owner of the mailbox"); + } else { + throw new MailboxNotFoundException(mailbox.getMailboxId()); + } + } } @VisibleForTesting diff --git a/server/protocols/jmap-draft-integration-testing/jmap-draft-integration-testing-common/src/test/java/org/apache/james/jmap/draft/methods/integration/cucumber/SetMailboxesMethodStepdefs.java b/server/protocols/jmap-draft-integration-testing/jmap-draft-integration-testing-common/src/test/java/org/apache/james/jmap/draft/methods/integration/cucumber/SetMailboxesMethodStepdefs.java index 13547c0..b572d83 100644 --- a/server/protocols/jmap-draft-integration-testing/jmap-draft-integration-testing-common/src/test/java/org/apache/james/jmap/draft/methods/integration/cucumber/SetMailboxesMethodStepdefs.java +++ b/server/protocols/jmap-draft-integration-testing/jmap-draft-integration-testing-common/src/test/java/org/apache/james/jmap/draft/methods/integration/cucumber/SetMailboxesMethodStepdefs.java @@ -95,18 +95,40 @@ public class SetMailboxesMethodStepdefs { String mailboxId = mainStepdefs.getMailboxId(owner, mailboxName).serialize(); String requestBody = - "[" + - " [ \"setMailboxes\"," + - " {" + - " \"update\": {" + - " \"" + mailboxId + "\" : {" + - " \"sharedWith\" : { \"" + shareTo + "\" : " + rightsAsString(rights) + " }" + - " }" + - " }" + - " }," + - " \"#0\"" + - " ]" + - "]"; + "[" + + " [ \"setMailboxes\"," + + " {" + + " \"update\": {" + + " \"" + mailboxId + "\" : {" + + " \"sharedWith\" : { \"" + shareTo + "\" : " + rightsAsString(rights) + " }" + + " }" + + " }" + + " }," + + " \"#0\"" + + " ]" + + "]"; + httpClient.post(requestBody); + } + + @Given("^\"([^\"]*)\" shares \"([^\"]*)\" delegated mailbox \"([^\"]*)\" with rights \"([^\"]*)\" with \"([^\"]*)\"$") + public void shareMailboxWithRight(String connectedUser, String owner, String mailboxName, String rights, String shareTo) throws Throwable { + userStepdefs.connectUser(connectedUser); + + String mailboxId = mainStepdefs.getMailboxId(owner, mailboxName).serialize(); + + String requestBody = + "[" + + " [ \"setMailboxes\"," + + " {" + + " \"update\": {" + + " \"" + mailboxId + "\" : {" + + " \"sharedWith\" : { \"" + shareTo + "\" : " + rightsAsString(rights) + " }" + + " }" + + " }" + + " }," + + " \"#0\"" + + " ]" + + "]"; httpClient.post(requestBody); } diff --git a/server/protocols/jmap-draft-integration-testing/jmap-draft-integration-testing-common/src/test/resources/cucumber/sharing/MailboxCreationAndSharing.feature b/server/protocols/jmap-draft-integration-testing/jmap-draft-integration-testing-common/src/test/resources/cucumber/sharing/MailboxCreationAndSharing.feature index 9083333..0aa4081 100644 --- a/server/protocols/jmap-draft-integration-testing/jmap-draft-integration-testing-common/src/test/resources/cucumber/sharing/MailboxCreationAndSharing.feature +++ b/server/protocols/jmap-draft-integration-testing/jmap-draft-integration-testing-common/src/test/resources/cucumber/sharing/MailboxCreationAndSharing.feature @@ -26,6 +26,10 @@ Feature: Mailbox creation and sharing And "al...@domain.tld" has a mailbox "shared" And "al...@domain.tld" shares her mailbox "shared" with "b...@domain.tld" with "aeilrwt" rights + Scenario: A sharee should not be able to update shared mailbox rights + When "b...@domain.tld" shares "al...@domain.tld" delegated mailbox "shared" with rights "aeilrwt" with "b...@domain.tld" + Then mailbox "shared" owned by "al...@domain.tld" is not updated + Scenario: A sharee should not be able to create a shared mailbox child Given "b...@domain.tld" creates mailbox "sharedChild" with creationId "c-01" in mailbox "shared" owned by "al...@domain.tld" When "al...@domain.tld" lists mailboxes --------------------------------------------------------------------- To unsubscribe, e-mail: server-dev-unsubscr...@james.apache.org For additional commands, e-mail: server-dev-h...@james.apache.org