-
Notifications
You must be signed in to change notification settings - Fork 9
IMAP, EWS, OWA: Allow others to access my mail folders #404 #885
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
base: master
Are you sure you want to change the base?
Conversation
307f3cc to
8703d72
Compare
|
Now with OWA support for folder and calendar permissions (OWA doesn't seem to support delegates). |
8703d72 to
17c0c7e
Compare
|
Now with support for addressbook permissions. Unfortunately due to a typo on my part I've had to work around a bug in #780... |
Sorry: But the UI doesn't have to be perfect, I can fix it later. |
|
| <ParticipantConfirmText {participant} /> | ||
| </hbox> | ||
| <hbox slot="result-bottom-row" let:person> | ||
| <PersonAvailability {person} /> |
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.
Could you please explain why you change this? It removes the Availability display.
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.
You're right, this doesn't belong in this PR; I was just putting in {person.emailAddress} everywhere, since it really frustrates me to see 10 contacts named "Ben" without knowing which one is the email address I want.
As it happens, <PersonAvailability> is just a stub. Instead Availability is shown further down using <AvailabilityGrid>.
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.
it really frustrates me to see 10 contacts named "Ben" without knowing which one is the email address I want.
Yes, I discussed that with @elenaux and we plan to add the email address to the results.
| export let tabindex = null; | ||
| export let autofocus = false; | ||
| export let typedText: string = ""; /* in/out */ | ||
| export let onAddPerson: (person: PersonUID) => void | Promise<void>; |
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.
Please make these Autocomplete event handlers changes a separate PR with its own justification.
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.
Indeed, I think this is a better solution for #826, so I'll switch that PR over to this method. (But it would need to land before I could rebase the changes away.)
| <hbox slot="result-bottom-row" class="recipient-email-address font-small" let:person> | ||
| {person.emailAddress} | ||
| </hbox> | ||
| <CalendarExchangePermissions slot="person-popup-bottom" let:person {person}/> |
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.
UI should be independent of Exchange. We need the same feature and UI in JMAP and even in IMAP accounts. IMAP supports this, and we have a specific request to implement that for Dovecot.
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.
Actually this section is for account permissions i.e. who has access to your Exchange Calendar "account" or Addressbook "account". The folder permissions are inapp/frontend/Settings/Mail/Account/ but I'd have to look at how folder permissions work in IMAP and JMAP to see whether I could unify the UI at all.
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.
IMAP rights work differently to Exchange folder permissions, so I can't really unify them. (They work more like Exchange delegate permissions, in that for instance the "Remove" action is immediate.) I tried to find out how JMAP mailbox permissions work, but I failed.
17c0c7e to
e20494e
Compare
e2f25c4 to
03715b0
Compare
|
I have written some shaky IMAP code to handle folder permissions.
|
|
You asked how folder permissions compare between IMAP and Exchange. IMAPYou either change the permissions as you go, or you calculate the changes between the original permissions and the permissions you want although this may require multiple commands. The following permissions are available:
ExchangeYou just set the final permissions for all permitted users in one go, overwriting all previous permission settings for that folder. (But I guess you could do that at every change the user makes instead of having a "Save Permissions" button.) I used the simple permissions model, but extended permissions are also available:
There is a table that maps the simple permissions to extended permissions:
(for Calendars, you can also have FreeBusyTimeOnly and FreeBusyTimeAndSubjectAndLocation which work like Reviewer but at a lower ReadItems permission) |
7d2a658 to
3640c13
Compare
This fixes a bug "introduced" by eaf6fe3. This PR now uses the approach from PR #885 of switching from an event to a callback. The default callback simply adds or removes the person from the collection, but Calendar can override the `onAddPerson()` to add a `Participant` instead, thus preserving type safety. Meanwhile PR #885 also wants to be able to override the `onRemovePerson()` callback as removing delegates happens immediately (as opposed to removing permissions which happens once editing of the permissions is complete).
|
@NeilRashbrook Now that #826 landed, this now conflicts. Could you please resolve them? Shouldn't be difficult for you: Just a few lines, and you know what you intended here. |
03715b0 to
3b751dd
Compare
app/logic/Mail/IMAP/IMAPFolder.ts
Outdated
| let permissions = new ArrayColl<IMAPPermission>(); | ||
| await conn.exec('GETACL', [{ type: 'ATOM', value: this.path }], { untagged: { async ACL(untagged) { attributes = untagged.attributes; } } }); | ||
| for (let i = 0; i < attributes.length; i += 2) { | ||
| let name = attributes[i].value; |
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.
sanitize() everything
| a = "owner", | ||
| } | ||
|
|
||
| export class IMAPPermission extends PersonUID { |
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.
An IMAPPermission should have a person, not be a person.
| export class IMAPPermission extends PersonUID { | |
| export class IMAPPermission { | |
| for: PersonUID; |
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.
Well, we'll see what what your final UI does, but for now, I'm using PersonsAutocomplete to hold the permitted users, and that needs the permissions to derive from, not aggregate, PersonUID.
| return permissions; | ||
| } | ||
|
|
||
| async setPermission(permission: IMAPPermission, right: keyof typeof IMAPACL) { |
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.
left: is missing, right?
| FreeBusyTimeAndSubjectAndLocation = 1 << 6, | ||
| } | ||
|
|
||
| export class ExchangePermission extends PersonUID { |
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.
See IMAP: for: PersonUID. It is not a Person.
| export class ExchangePermission extends PersonUID { | |
| export class ExchangePermission { | |
| for: PersonUID; |
app/logic/Mail/EWS/EWSFolder.ts
Outdated
| } | ||
| } | ||
|
|
||
| export enum ExchangePermissions { |
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.
We need to find a way to unify that with IMAPPermissions
| } | ||
| } | ||
|
|
||
| export enum IMAPACL { |
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.
We need to find a way to unify that with ExchangePermissions
3b751dd to
9739c37
Compare
9739c37 to
cf0ee4c
Compare
|
Now updated to present six of the Exchange folder permissions for individual editing. |
| }, { t$CalendarPermissionLevel: "Custom" }, this.exchangePermissions.toEWS()); | ||
| } | ||
|
|
||
| toOWAFolderPermission() { |
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.
OWA code in EWS?
The general direction is a generic API for permissions that works across all protocols.
|
I've just noticed that my UI has a bug - it will try to display delegates of delegated accounts, which doesn't work. |
|
More bugs in my UI:
|
*UI doesn't report errors reporting setting IMAP permissions, also connection becomes unusable if an error occurs, but I don't know why |
|
Thanks, Neil, for your work on this. FWIW, the individual permissions should reflect the protocol level as closely as possible, after abstracting the different protocols. The options in the UI for the individual permissions were incomplete. It should have "create subfolder", "rename folder" etc. Just please unify them between Exchange, IMAP and JMAP, as we discussed, so that the individual flags work across the protocols. We don't need to reflect admin nor owner access levels. And for Exchange, we accept that "edit flags" also means "can rewrite message content" and just document that in the JSDoc (and I'll add something to the UI later). |
3cbc792 to
9abd046
Compare


Although #404 claimed that UX mockups exist it doesn't actually help because I don't know where they are, so I wrote some basic UI.
Permissions are entered using the autocomplete UI. You can add users using their email address and click on a user to edit their permissions. In the case of delegated users, each user is edited and saved separately using a button inside the popup, while in the case of folder and calendar permissions, these are all saved in one go using a separate button.
So that the easy permission UI can be slightly more easily understood, some of their effective permissions are shown as a group of checkboxes, but these are display only.