-
Notifications
You must be signed in to change notification settings - Fork 473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
James-4097 Allow disabling same-domain requirement when assigning rights #2573
James-4097 Allow disabling same-domain requirement when assigning rights #2573
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks overall very good indeed ;-)
Ex in `jvm.properties` | ||
---- | ||
james.rights.users.crossdomain=false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about naming this
james.rights.crossdomain.allow=false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that!
@@ -74,6 +74,10 @@ public StoreRightManager(MailboxSessionMapperFactory mailboxSessionMapperFactory | |||
this.eventBus = eventBus; | |||
} | |||
|
|||
private boolean isCrossDomainAccessAllowed() { | |||
return Boolean.parseBoolean(System.getProperty("james.rights.users.crossdomain", "false")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it a constant: looking this up at class loading is enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a good idea 👍
mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreRightManager.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
James-4097
Add a configuration switch lifting the same-domain requirement when assigning rights.
This feature is opt-in - the default configuration will still forbid cross-domain access.