If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

IMAP ACL edit (Backend): implement setacl

ASSIGNED
Assigned to

Status

MailNews Core
Networking: IMAP
--
enhancement
ASSIGNED
8 years ago
3 years ago

People

(Reporter: BenB, Assigned: BenB)

Tracking

(Blocks: 2 bugs)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 10 obsolete attachments)

(Assignee)

Description

8 years ago
Implement setacl IMAP protocol feature (RFC 4314
<http://www.apps.ietf.org/rfc/rfc4314.html#sec-3.1>) in IMAP backend.
(Assignee)

Updated

8 years ago
Blocks: 522955
(Assignee)

Comment 1

8 years ago
Created attachment 406915 [details] [diff] [review]
Fix, v3, by bienvenu

David Bienvenu created and sent me this patch.

I read over the code and it makes sense to me.
I tested it, and it works well for me.

(I only changed the patch to allow an empty "rights" string, to allow to set the ACL back to "no access".)

Outstanding: Refresh the ACL (from the server) after we set it. Implementation: Just trigger a "getacl" server command - the existing code will parse it and put it in the internal data structures. This should be completed before the callback passed to SetFolderACL() is called - I'll then refresh the UI.
Attachment #406915 - Flags: review+

Comment 2

8 years ago
dupe of bug  Bug 207628 ?
(Assignee)

Comment 3

8 years ago
Nikolay, thanks for the pointer. I guess there was bound to be a bug for this already :). That bug is for the UI, though, which would be bug 522955.
(Assignee)

Updated

8 years ago
Blocks: 207628
(Assignee)

Updated

8 years ago
Summary: Allow to change the IMAP ACL = share folders with others (Backend) → IMAP ACL edit (Backend): implement setacl
(Assignee)

Comment 4

8 years ago
Created attachment 407058 [details] [diff] [review]
Fix, v4

Fix v3 plus
- new function refreshACL()
- rename nsIMsgImapMailFolder::setFolderACL() to ::setACL()
  (nsIImapService keeps setFolderACL())
- use NS_ENSURE_SUCCESS instead of nested |if|s.
- Update comments.
Attachment #406915 - Attachment is obsolete: true
Attachment #407058 - Flags: review?(bienvenu)
(Assignee)

Comment 5

8 years ago
Created attachment 407059 [details] [diff] [review]
Fix, v4
Attachment #407058 - Attachment is obsolete: true
Attachment #407058 - Flags: review?(bienvenu)
(Assignee)

Comment 6

8 years ago
Comment on attachment 407059 [details] [diff] [review]
Fix, v4

Neil, this patch is mostly from bienvenu, with the refreshACL() part from me. bienvenu said you'd be a good (super)reviewer for this.
Attachment #407059 - Flags: superreview?(neil)
Attachment #407059 - Flags: review?(neil)

Comment 7

8 years ago
Comment on attachment 407059 [details] [diff] [review]
Fix, v4

This seems reasonable from an sr point of view but I can't really review it.

>+   *        Special values (not yet implemented):
>+   *        "*self*" = the current user
>+   *               Use with care to not deny yourself access.
>+   *        "*public*" = all authenticated users
The RFC I read already specifies the special value "anyone" for this.

>+  if (aModifier < nsIMsgImapMailFolder::addRights ||
>+      aModifier > nsIMsgImapMailFolder::replaceRights)
Nit: could use NS_ENSURE_ARG_RANGE

>+  nsCString escapedName;
>+  CreateEscapedMailboxName(mailboxName, escapedName);
>+  command.Append(" setacl \"");
>+  command.Append(escapedName);
>+  command.Append("\" ");
>+  nsCString userName;
>+  m_runningUrl->GetAclUsername(userName);
>+  nsCString aclRights;
>+  m_runningUrl->GetAclRights(aclRights);
>+  command.Append(userName);
>+  command.Append(" \"");
>+  command.Append(aclRights);
Do the user name and rights need to be escaped (or at least sanitised)?

>+  urlSpec.Append('>');
>+  urlSpec.Append(aUsername);
>+  urlSpec.Append('>');
>+  if (aModifier == nsIMsgImapMailFolder::addRights)
>+    urlSpec.Append('+');
>+  else if (aModifier == nsIMsgImapMailFolder::removeRights)
>+    urlSpec.Append('-');
>+  urlSpec.Append(aRights);
[And again escaped or sanitised here.]

>+void nsImapUrl::ParseUsernameAndRights()
>+{
>+  if (m_tokenPlaceHolder)
>+  {
>+    m_username.Assign(NS_strtok(IMAP_URL_TOKEN_SEPARATOR, &m_tokenPlaceHolder));
>+    if (m_tokenPlaceHolder)
>+      m_rights.Assign(NS_strtok(IMAP_URL_TOKEN_SEPARATOR, &m_tokenPlaceHolder));
[And then unescaped here if necessary.]
Attachment #407059 - Flags: review?(neil)

Comment 8

8 years ago
the rights are a very straightforward set of 11 letters and don't need to be escaped; The username could conceivably need to be escaped, especially if '>' is allowed. The user name is defined as simply an imap string in the rfc, i.e., astring.

Comment 9

8 years ago
(In reply to comment #8)
> the rights are a very straightforward set of 11 letters
But I don't see any validation of them in the code...

Comment 10

8 years ago
I was just saying they don't need to be escaped...if the caller passes in bad rights, the server should fail trying to set the acl.
(Assignee)

Updated

8 years ago
Attachment #407059 - Flags: superreview?(neil) → review?(neil)
(Assignee)

Updated

8 years ago
Attachment #407059 - Flags: superreview?(neil)
Attachment #407059 - Flags: review?(neil)
Attachment #407059 - Flags: review?(bugzilla)
(In reply to comment #10)
> I was just saying they don't need to be escaped...if the caller passes in bad
> rights, the server should fail trying to set the acl.
I was thinking more along the lines of special characters... what if the rights string contains spaces or > signs?
(Assignee)

Comment 12

8 years ago
Yup, good point. I'll update the patch to URL-escape and unescape the username in our own IMAP URL.
bienvenu, does IMAP support escaping? What's the delimiter here? Space, right?

Comment 13

8 years ago
the IMAP protocol supports imap mod- utf7 - it doesn't support escaping or unescaping, in the sense that you send an escaped string to the server and it unescapes it.  It uses " " delimited strings whenever it needs to worry about escaping. \" escapes ".

If the username can contain slashes, you may need to escape them as well when creating the url, like we do in nsImapService::RenameLeaf

      nsCAutoString utfNewName;
      CopyUTF16toMUTF7(nsDependentString(newLeafName), utfNewName);
      char* escapedNewName = nsEscape(utfNewName.get(), url_Path);
      NS_ENSURE_TRUE(escapedNewName, NS_ERROR_OUT_OF_MEMORY);
      nsCString escapedSlashName;
      rv = nsImapUrl::EscapeSlashes(escapedNewName, getter_Copies(escapedSlashName));
      NS_ENSURE_SUCCESS(rv, rv);
      NS_Free(escapedNewName);
      urlSpec.Append(escapedSlashName);
Comment on attachment 407059 [details] [diff] [review]
Fix, v4

As we don't have any UI for this, I'd be much happier reviewing it if it has unit test to go with it - especially as I see no discussion in the interfaces about error states or ACL change failures.

The imap fake server should be easy to extend for something like this.
(Assignee)

Comment 15

8 years ago
> As we don't have any UI for this

We do. There is complete, finished UI for this in bug 522955. I plan to get this into TB, putting the already existing (but not very functional) Sharing tab into good use, once the backend is done.
Comment on attachment 407059 [details] [diff] [review]
Fix, v4

Sorry, but without tests there's no easy way to verify that this is doing even approximately what it needs to, and no regression testing facilities.

Joshua and I should be able to give you pointers on fake servers if you need them.
Attachment #407059 - Flags: review?(bugzilla) → review-
(Assignee)

Comment 17

8 years ago
> there's no easy way to verify that this is doing even
> approximately what it needs to

Sure there is. Just install the extension and try it out in the GUI.

You'd see the effects even if you have only one user account at the IMAP server. I can give you several test accounts on an IMAP server, too, to try it out end-to-end.

Can we please have a proper code review before you r- minus it?
Attachment #407059 - Flags: review- → review?(bugzilla)
Comment on attachment 407059 [details] [diff] [review]
Fix, v4

I'll put it back in my queue, however note that my queue is large at the moment and my extra TB 3 work means it may be a week or two before I have time to do a code review on it.
Ben: note that we're moving towards requiring _automated_ tests for the vast majority of changes.  As this code is not likely to be tested by most testers on an ongoing basis, I think it is pretty much guaranteed to break over time if it's not covered by automated tests.
(Assignee)

Comment 20

8 years ago
Yup, I'm planning to do this for this bug, but it will take some time for me to get acquainted with the test framework.
(Assignee)

Updated

8 years ago
Attachment #407059 - Flags: superreview?(neil)
Attachment #407059 - Flags: review?(bugzilla)
(Assignee)

Comment 21

8 years ago
Created attachment 429215 [details] [diff] [review]
Fix, v5

(still not escaping username)
Assignee: bienvenu → ben.bucksch
Attachment #407059 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #429215 - Flags: review?
(Assignee)

Comment 22

8 years ago
Created attachment 429217 [details] [diff] [review]
Tests, v3

Automated tests for this.
Attachment #429217 - Flags: review?(bwinton)
(Assignee)

Comment 23

8 years ago
Created attachment 429226 [details] [diff] [review]
Tests, v4
Attachment #429226 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #429226 - Flags: review? → review?(bugzilla)
(Assignee)

Updated

8 years ago
Attachment #429217 - Attachment is obsolete: true
Attachment #429217 - Flags: review?(bwinton)
(Assignee)

Comment 24

8 years ago
Created attachment 433384 [details] [diff] [review]
Fix, v7, incl. tests

Properly escaped the username.
The username is passed as cstring, so I t hink no need to convert UTF16 to M-UTF-7.
As said above, the rights string is just a set of flags (which are ASCII), so no need to espace (otherwise wrong API use).

Please review. Although bienvenu created the initial version of the code I worked on it a lot, so I think it's alright when he reviews and somebody else does sr, or vice versa.
Attachment #429215 - Attachment is obsolete: true
Attachment #429226 - Attachment is obsolete: true
Attachment #433384 - Flags: superreview?(neil)
Attachment #433384 - Flags: review?(bienvenu)
Attachment #429215 - Flags: review?
Attachment #429226 - Flags: review?(bugzilla)
(Assignee)

Comment 25

8 years ago
ping: reviews

Comment 26

8 years ago
Comment on attachment 433384 [details] [diff] [review]
Fix, v7, incl. tests

Have not run this yet, but some quick comments:

need doxygen comments for this:

+  /**
+   * Ask the server about the current ACL again.
+   */
+  nsIURI refreshFolderACL(in nsIEventTarget aClientEventTarget,

use MsgEscapeString instead of nsEscape, and MsgUnescape...

indentation issue:

+  nsIURI setACL(in ACString aUsername, in long aModifier,
+                         in ACString aRights,
+                         in nsIUrlListener aListener);
(Assignee)

Comment 27

8 years ago
Created attachment 438824 [details] [diff] [review]
Fix, v8

- Msg(Un)EscapeString
- Fixed indention
- Improved comments
Attachment #433384 - Attachment is obsolete: true
Attachment #438824 - Flags: superreview?(neil)
Attachment #438824 - Flags: review?(bienvenu)
Attachment #433384 - Flags: superreview?(neil)
Attachment #433384 - Flags: review?(bienvenu)

Comment 28

8 years ago
Comment on attachment 433384 [details] [diff] [review]
Fix, v7, incl. tests

this has bit-rotted, in the sense that it breaks several tests now - test_imapPump.js for one, and a couple others
Attachment #433384 - Flags: review-

Comment 29

8 years ago
Comment on attachment 438824 [details] [diff] [review]
Fix, v8

can you update and re-run the tests? This seems to have broken several tests, which I think is a bit-rot thing:

TEST-UNEXPECTED-FAIL | C:\mozilla-build\msys\tbirdhq\objdir-tb\mozilla\_tests\xp
cshell\test_mailnewsbase\unit\test_imapPump.js | test failed (with xpcshell retu
rn code: 3), see following log:
TEST-UNEXPECTED-FAIL | C:\mozilla-build\msys\tbirdhq\objdir-tb\mozilla\_tests\xp
cshell\test_addbook\unit\test_bug534822.js | test failed (with xpcshell return c
ode: 0), see following log:
TEST-UNEXPECTED-FAIL | C:/mozilla-build/msys/tbirdhq/objdir-tb/mozilla/_tests/xp
cshell/test_addbook/unit/test_bug534822.js | false == true - See following stack
:
TEST-UNEXPECTED-FAIL | C:\mozilla-build\msys\tbirdhq\objdir-tb\mozilla\_tests\xp
cshell\test_imap\unit\test_preserveDataOnMove.js | test failed (with xpcshell re
turn code: 3), see following log:

Comment 30

8 years ago
Comment on attachment 438824 [details] [diff] [review]
Fix, v8

minus based on test breakage. However, modulo that, and my previous comments, this patch looks OK.
Attachment #438824 - Flags: review?(bienvenu) → review-

Comment 31

8 years ago
sorry, bit of a collision there - but you should update your tree and see if the unit tests pass for you...
(Assignee)

Comment 32

8 years ago
Created attachment 438864 [details] [diff] [review]
Fix, v9
Attachment #438824 - Attachment is obsolete: true
Attachment #438864 - Flags: superreview?(neil)
Attachment #438864 - Flags: review?(bienvenu)
Attachment #438824 - Flags: superreview?(neil)
(Assignee)

Comment 33

8 years ago
Created attachment 438869 [details] [diff] [review]
Fix, v10
Attachment #438864 - Attachment is obsolete: true
Attachment #438869 - Flags: superreview?(neil)
Attachment #438869 - Flags: review?(bienvenu)
Attachment #438864 - Flags: superreview?(neil)
Attachment #438864 - Flags: review?(bienvenu)

Comment 34

8 years ago
Comment on attachment 438869 [details] [diff] [review]
Fix, v10

ah, I bet you didn't want the ldap stuff in this patch.
(Assignee)

Comment 35

8 years ago
Indeed not. Just pretend it's not there :).
(Assignee)

Comment 36

8 years ago
Created attachment 438908 [details] [diff] [review]
Fix, v11

LDAP stuff manually edited out
Attachment #438869 - Attachment is obsolete: true
Attachment #438908 - Flags: superreview?(neil)
Attachment #438908 - Flags: review?(bienvenu)
Attachment #438869 - Flags: superreview?(neil)
Attachment #438869 - Flags: review?(bienvenu)

Updated

8 years ago
Attachment #438908 - Flags: review?(bienvenu) → review+
(Assignee)

Comment 37

8 years ago
Comment on attachment 438908 [details] [diff] [review]
Fix, v11

neil: ping sr
Comment on attachment 438908 [details] [diff] [review]
Fix, v11

>+  return imapService->SetFolderACL(NS_GetCurrentThread(),
...
>+  return imapService->RefreshFolderACL(NS_GetCurrentThread(),
Sadly NS_GetCurrentThread() is internal-API only. You'll need to change this to nsCOMPtr<nsIThread> thread(do_GetCurrentThread()); sr=me with that fixed.

>-  nsCString hostname;
>+  nsCString hostname; // TODO use nsCAutoString
[Why?]

>-    *((char **)getter_Copies(escapedUsername)) = nsEscape(username.get(), url_XAlphas);
>+    *((char **)getter_Copies(escapedUsername)) = nsEscape(username.get(),
>+          url_XAlphas); // TODO use MsgEscapeString (other places, too)
Hopefully bug 377319 will fix this, please don't bitrot us by adding comments!

>+    char* username = PL_strdup(NS_strtok(IMAP_URL_TOKEN_SEPARATOR,
>+                                         &m_tokenPlaceHolder));
>+    MsgUnescapeString(nsDependentCString(username), 0, m_username);
>+    PL_strfree(username);
No need to strdup. const char* username = NS_strtok(... suffices.
Attachment #438908 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 39

7 years ago
Will update patch and commit (for 3.2)

Comment 40

7 years ago
Very cool that this is coming...

Is anyone testing it with a dovecot server on the backend?

Hopefully even a 2.0beta build?
You need to log in before you can comment on or make changes to this bug.