chibenwa commented on code in PR #2573:
URL: https://github.com/apache/james-project/pull/2573#discussion_r1892093312
##########
docs/modules/servers/partials/configure/jvm.adoc:
##########
@@ -172,4 +172,17 @@ james.deduplicating.blobstore.thread.switch.threshold=32768
# Count of octet from which streams are buffered to files and not to memory
james.deduplicating.blobstore.file.threshold=10240
-----
\ No newline at end of file
+----
+
+== Allow users to have rights for shares of different domain
+
+Typically, preventing users to obtain rights for shares of another domain is a
useful security layer.
+However, in multi-tenancy deployments, this can be useful (for example,
students might be given access to a shared mailbox
+residing under the @university.edu domain with their @student.university.edu
address).
+
+Optional. Boolean. Defaults to false.
+
+Ex in `jvm.properties`
+----
+james.rights.users.crossdomain=false
Review Comment:
How about naming this
```
james.rights.crossdomain.allow=false
```
?
##########
mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreRightManager.java:
##########
@@ -74,6 +74,10 @@ public StoreRightManager(MailboxSessionMapperFactory
mailboxSessionMapperFactory
this.eventBus = eventBus;
}
+ private boolean isCrossDomainAccessAllowed() {
+ return
Boolean.parseBoolean(System.getProperty("james.rights.users.crossdomain",
"false"));
Review Comment:
Make it a constant: looking this up at class loading is enough
##########
mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreRightManager.java:
##########
@@ -185,7 +189,7 @@ public void applyRightsCommand(MailboxPath mailboxPath,
ACLCommand mailboxACLCom
@Override
public Mono<Void> applyRightsCommandReactive(MailboxPath mailboxPath,
ACLCommand mailboxACLCommand, MailboxSession session) {
return Mono.just(mailboxSessionMapperFactory.getMailboxMapper(session))
- .doOnNext(Throwing.consumer(mapper ->
assertSharesBelongsToUserDomain(mailboxPath.getUser(), mailboxACLCommand)))
+ .doOnNext(Throwing.consumer(mapper ->
assertUserHasAccessToShareDomains(mailboxPath.getUser(), mailboxACLCommand)))
Review Comment:
```suggestion
.doOnNext(Throwing.consumer(mapper ->
assertUserHasAccessToShareeDomains(mailboxPath.getUser(), mailboxACLCommand)))
```
##########
mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreRightManager.java:
##########
@@ -74,6 +74,10 @@ public StoreRightManager(MailboxSessionMapperFactory
mailboxSessionMapperFactory
this.eventBus = eventBus;
}
+ private boolean isCrossDomainAccessAllowed() {
+ return
Boolean.parseBoolean(System.getProperty("james.rights.users.crossdomain",
"false"));
Review Comment:
Ah ok I got it, that's needed because of the tests!
My suggestion is to allow the constant to be package local mutable annotated
with `@VisibleForTesting` so that overloading that in tests become easy while
keeping the property lookup at classloading time.
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
