522954 - IMAP ACL edit (Backend): implement setacl

Status: ASSIGNED

(MailNews Core :: Networking: IMAP, enhancement)

Description

[Ben Bucksch (:BenB)]

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

Comment 1

[Ben Bucksch (:BenB)]

Attachment: 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.

Comment 2

[Nikolay Shopik]

dupe of bug  Bug 207628 ?

Comment 3

[Ben Bucksch (:BenB)]

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.

Comment 4

[Ben Bucksch (:BenB)]

Attachment: 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.

Comment 5

[Ben Bucksch (:BenB)]

Attachment: Fix, v4

Comment 6

[Ben Bucksch (:BenB)]

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.

Comment 7

[[e-mail]]

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.]

Comment 8

[David :Bienvenu]

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

[[e-mail]]

(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

[David :Bienvenu]

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.

Comment 11

[[e-mail]]

(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?

Comment 12

[Ben Bucksch (:BenB)]

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

[David :Bienvenu]

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 14

[Mark Banner (:standard8)]

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.

Comment 15

[Ben Bucksch (:BenB)]

> 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 16

[Mark Banner (:standard8)]

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.

Comment 17

[Ben Bucksch (:BenB)]

> 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?

Comment 18

[Mark Banner (:standard8)]

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.

Comment 19

[David Ascher (:davida)]

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.

Comment 20

[Ben Bucksch (:BenB)]

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.

Comment 21

[Ben Bucksch (:BenB)]

Attachment: Fix, v5

(still not escaping username)

Comment 22

[Ben Bucksch (:BenB)]

Attachment: Tests, v3

Automated tests for this.

Comment 23

[Ben Bucksch (:BenB)]

Attachment: Tests, v4

Comment 24

[Ben Bucksch (:BenB)]

Attachment: 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.

Comment 25

[Ben Bucksch (:BenB)]

ping: reviews

Comment 26

[David :Bienvenu]

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);

Comment 27

[Ben Bucksch (:BenB)]

Attachment: Fix, v8

- Msg(Un)EscapeString
- Fixed indention
- Improved comments

Comment 28

[David :Bienvenu]

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

Comment 29

[David :Bienvenu]

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

[David :Bienvenu]

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.

Comment 31

[David :Bienvenu]

sorry, bit of a collision there - but you should update your tree and see if the unit tests pass for you...

Comment 32

[Ben Bucksch (:BenB)]

Attachment: Fix, v9

Comment 33

[Ben Bucksch (:BenB)]

Attachment: Fix, v10

Comment 34

[David :Bienvenu]

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

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

Comment 35

[Ben Bucksch (:BenB)]

Indeed not. Just pretend it's not there :).

Comment 36

[Ben Bucksch (:BenB)]

Attachment: Fix, v11

LDAP stuff manually edited out

Comment 37

[Ben Bucksch (:BenB)]

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

neil: ping sr

Comment 38

[[e-mail]]

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.

Comment 39

[Ben Bucksch (:BenB)]

Will update patch and commit (for 3.2)

Comment 40

[Charles]

Very cool that this is coming...

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

Hopefully even a 2.0beta build?