Closed Bug 1494584 Opened 2 years ago Closed 3 months ago

Use nsIMsgImapMailFolder.getOtherUsersWithAccess on IMAP's folder sharing tab

Categories

(MailNews Core :: Networking: IMAP, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 78.0

People

(Reporter: jorgk-bmo, Assigned: rnons)

References

Details

Attachments

(2 files, 5 obsolete files)

+++ This bug was initially created as a clone of Bug #1494238 +++

nsIMsgImapMailFolder.getOtherUsersWithAccess is working now so we can use it to show users with access rights to a folder.
Whiteboard: [good-first-bug]
Attached patch 1494584.patch (obsolete) — Splinter Review

This patch uses getOtherUsersWithAccess to render users on sharing tab. But somehow, getOtherUsersWithAccess() also returns the current user.

I setup Cyrus on my local to test IMAP sharing. In the case of FastMail, the myrights returned doesn't include a - administer right. So we can't send getacl command to check if a folder is shared. According to https://github.com/jmapio/jmap/issues/126#issuecomment-326860851, administer right is not supported by JMAP yet.

Attachment #9150977 - Flags: review?(mkmelin+mozilla)
Attached image IMAP_shared_folder.png (obsolete) —

It looks like this. rnons is the current user.

Attached patch 1494584.patch (obsolete) — Splinter Review

I realized "the current user is included" is due to my local config. I'm using rnons@localhost to login from Thunderbird, but the user inside Cyrus is named rnons. So it's not a real problem.

Attachment #9150977 - Attachment is obsolete: true
Attachment #9150977 - Flags: review?(mkmelin+mozilla)
Attachment #9150982 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9150982 [details] [diff] [review]
1494584.patch

Review of attachment 9150982 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/locales/en-US/chrome/messenger/folderProps.dtd
@@ +56,4 @@
>  <!ENTITY privileges.button.label                 "Privileges…">
>  <!ENTITY privileges.button.accesskey             "P">
>  <!ENTITY permissionsDesc.label                   "You have the following permissions:">
> +<!ENTITY folderOtherUsers.label                  "Folder accessible by:">

I'd suggest to word this as:

Others with access to this folder:

::: mailnews/base/content/folderProps.js
@@ +309,5 @@
> +    let iter = imapFolder.getOtherUsersWithAccess();
> +    let users = [];
> +    while (iter.hasMore()) {
> +      users.push(iter.getNext());
> +    }

you should be able to use for .. of for this. 

for (let user of imapFolder.getOtherUsersWithAccess()) 

Or maybe even spread simply as
 let users = [... imapFolder.getOtherUsersWithAccess() ];

@@ +311,5 @@
> +    while (iter.hasMore()) {
> +      users.push(iter.getNext());
> +    }
> +    document.getElementById("folderOtherUsers").hidden = false;
> +    document.getElementById("folderOtherUsers.text").textContent = users.join(

Please don't use periods in ids - it's a hazzle for css where it has a meaning

::: mailnews/base/content/folderProps.xhtml
@@ +173,4 @@
>  
>          <description id="folderPermissions.text"></description>
>        </vbox>
> +      <vbox align="start" id="folderOtherUsers" hidden="true">

nit: we prefer to put the id as first attribute
Attachment #9150982 - Flags: review?(mkmelin+mozilla)
Attached patch 1494584.patch (obsolete) — Splinter Review

Thanks, fixed now.

Attachment #9150982 - Attachment is obsolete: true
Attachment #9150992 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9150992 [details] [diff] [review]
1494584.patch

Review of attachment 9150992 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Unfortunately the backend doesn't seem to work 100% right - unclear to me if it's the server or our code that has a bug. At least for Fastmail, even if I share a folder I still get a "This folder is not shared" (I've shared one with you). It also lists (only) myself there, so it's a bit confusing. For folders shared with me, I'm listed under others.
Attachment #9150992 - Flags: review?(mkmelin+mozilla) → review+
Assignee: nobody → remotenonsense
Status: NEW → ASSIGNED

At least for Fastmail, even if I share a folder I still get a "This folder is not shared"

This is expected. As I mentioned in comment 2, the myrights returned by FastMail doesn't include a - administer right. So we can't send getacl command to check if a folder is shared. According to https://github.com/jmapio/jmap/issues/126#issuecomment-326860851, administer right is not supported by JMAP yet.

https://searchfox.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp#6245-6247, only when myrights contains a, GetACLForFolder is sent. GetACLForFolder will return all users of a folder.

It also lists (only) myself there, so it's a bit confusing.

Yes, I just noticed, so comment 4 was wrong. I will look into it.

But we're using IMAP, not JMAP. Anyway, yes please check what's up with that.

Attached patch 1494584.patch (obsolete) — Splinter Review

Added code to get the current user name, then filter it out in nsMsgIMAPFolderACL::GetOtherUsers, so that sharing tab doesn't contain oneself.

(In reply to Magnus Melin [:mkmelin] from comment #9)

But we're using IMAP, not JMAP. Anyway, yes please check what's up with that.

I was guessing Fastmail is using JMAP, I filed a support ticket to them and am waiting for their response.

Attachment #9150992 - Attachment is obsolete: true
Attachment #9151310 - Flags: review?(mkmelin+mozilla)
Attached image IMAP_shared_folder.png

Unlike comment 3, current user (rnons) is not listed now.

Attachment #9150979 - Attachment is obsolete: true

(In reply to Ping Chen (:rnons) from comment #2)

In the case of FastMail, the myrights returned doesn't include a - administer right. So we can't send getacl command to check if a folder is shared.

Right. https://tools.ietf.org/html/rfc4314#section-4

2020-05-24 11:36:02.388644 UTC - [(null) 7281: IMAP]: I/IMAP 0x7f0b5facf000:imap.fastmail.com:S-Shared:SendData: 13 myrights "Shared"
2020-05-24 11:36:02.499373 UTC - [(null) 7281: IMAP]: I/IMAP 0x7f0b5facf000:imap.fastmail.com:S-Shared:CreateNewLineFromSocket: * MYRIGHTS Shared lrswipkxtecdn

Seems like a Fastmail bug - this is my own folder!
I do, however, get a lrswipkxtecdan response for someone else's mailbox (which I have full access to)

(In reply to Ping Chen (:rnons) from comment #10)

I was guessing Fastmail is using JMAP

They support JMAP too yes. Thunderbird doesn't support JMAP yet though - this is all IMAP.

Comment on attachment 9151310 [details] [diff] [review]
1494584.patch

Review of attachment 9151310 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +5938,3 @@
>    nsTArray<nsCString> *resultArray = new nsTArray<nsCString>;
>    for (auto iter = m_rightsHash.Iter(); !iter.Done(); iter.Next()) {
> +    if (myUserName != iter.Key())

I wonder if this should be using Equals instead. 
Seems to work though.
Attachment #9151310 - Flags: review?(mkmelin+mozilla) → review?

I wasn't so sure, so I searched for nsCString on MDN, then found https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Glue_classes/nsCString#Equals and https://hg.mozilla.org/mozilla-central/file/068446ee5493/xpcom/string/public/nsTSubstring.h#l686

inline
PRBool operator!=( const nsTSubstring_CharT::base_string_type& lhs, const nsTSubstring_CharT::base_string_type& rhs )
  {
    return !lhs.Equals(rhs);
  }

Note that BMO does strange things these days like "Attachment #9151310 [details] [diff] - Flags: review?(mkmelin+mozilla@iki.fi) → review?" when you comment on a patch.

Yes, Mangus (and I) prefer(s) the "old school" .Equals(), but the operators exist:
https://searchfox.org/mozilla-central/rev/501eb4718d73870892d28f31a99b46f4783efaa0/xpcom/string/nsTStringRepr.h#390

Attached patch 1494584.patchSplinter Review

Changed to use .Equals.

Attachment #9151310 - Attachment is obsolete: true
Attachment #9151310 - Flags: review?
Attachment #9151374 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9151374 [details] [diff] [review]
1494584.patch

Review of attachment 9151374 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! r=mkmelin
Attachment #9151374 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → Thunderbird 78.0

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/21be6632c274
show other users on IMAP folder sharing tab. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED

Just for further reference: Fastmail are aware of the missing "a" and will fix it, but there is no timeline for a fix.

You need to log in before you can comment on or make changes to this bug.