-
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-2182 Sharing for IMAP #2445
base: master
Are you sure you want to change the base?
Conversation
Edit: evening coding session, most spotted bugs and a few others are out of the way but extensive testing will be required. |
b21f6de
to
a3fcbd9
Compare
1baac9e
to
ba95f71
Compare
We do not need those IMAP concerns onto the storage layer. We will later come up with alternative implementation for namespace customization.
Guice implementors can then very easily overwrite the namespace definition
This will allow customization of the IMAP folder naming strategy.
…-> mailboxname - Centralize in one place the knowledge of encoding MailboxPath into IMAP
As we can only access resources within a single domain the domain is implicit and thus do not need to be repeated in the IMAP wording.
This common character conflicts with the path delimiter of the mailbox name and thus needs to be escaped.
Several issues remains to address: - [ ] List + myrights needs to keep FQDN in the response - [ ] #user shall not include personal mailboxes filter them out? (post filtering?) - [ ] Using #user in mailbox name pattern shall be permitted - [ ] Remove duplicate results...
ba95f71
to
babafcd
Compare
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.
Quite a big one to read, but interesting. Huge work here, cool!
Just a minor remark but overall LGTM
mailbox/store/src/main/java/org/apache/james/mailbox/store/StoreMailboxManager.java
Show resolved
Hide resolved
if (!isWriteable(mailboxSession)) { | ||
throw new ReadOnlyException(getMailboxPath()); | ||
if (flagsUpdateMode.equals(FlagsUpdateMode.REPLACE)) { | ||
if (!myRights.contains(MailboxACL.Right.Write, MailboxACL.Right.WriteSeenFlag, MailboxACL.Right.DeleteMessages)) { |
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.
Maybe containsAll? I had to go into the contain method to verify that all input rights have to be contained.
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 it's rather explicit, and it is unrelated with this change set.
Maybe others have opinions?
protocols/imap/src/main/java/org/apache/james/imap/main/PathConverter.java
Show resolved
Hide resolved
protocols/imap/src/main/java/org/apache/james/imap/processor/NamespaceSupplier.java
Show resolved
Hide resolved
…ion to PathConverter This allows customization...
0455d60
to
ff04821
Compare
My second shot ever at this.
First a BIG refactoring centralizes the definition of path <-> name convertion into the IMAP layer (pre-requisite) and
provide a generic strategy that could be allowed to be overloaded (PathConverter + Namespace supplier).
A second part of this work is about exposing other users folder shared with the user into a
#user
folder.Remaining work:
I am happy, in 1 day coding it got quite far, though details needs to be addressed and more tests needs to be produced.