112105 - Clicking on a folder link should allow subscribing to shared folder

Status: VERIFIED FIXED

(MailNews Core :: Networking: IMAP, defect, P2)

Description

[scottputterman]

From the shared folders engineering plan:

IMAP url generating and parsing needs to be implemented so that one user can
copy a link to a folder into a mail message, and the recepient can click on the
message to subscibe to the folder. This is the pimary way that people subscribe
to shared folders.

Comment 1

[David :Bienvenu]

In order to do this, we'd need the ability to generate such a url. In 4.x, this
was done with the folder link button, but that was dropped in 6.x. I can write
code that makes clicking on an imap link to a shared folder subscribe you to
that folder, but it's useless unless there's some way for the user to generate
such a link. Is there a proposal for a ui to generate links to folders? The
simplest approach would probably be to add a context menu item to the folder
context menu, "copy folder location", which would be consistent with what we do
in the message pane to create a link to a message, "copy message location"

Comment 2

[David :Bienvenu]

copy message location only seems to be implemented for news messages.

Comment 3

[David :Bienvenu]

OK, I'm getting old AND senile. The folder link widget was only in 4.0x, not in
4.5+ - to create imap folder urls in 4.5+, you would select the folder in the
folder pane and drag drop it into a compose window.

Comment 4

[scottputterman]

A context menu item sounds easiest to me.  Once we support this url, what will
happen if you paste it into the urlbar in the browser or if you click on a link
that doesn't exist or you don't have access to?

Comment 5

[David :Bienvenu]

I'm thinking we should do drag drop as well, if it's not too hard (my guess is
that it's either trivial or impossible, and it won't take too long to discover
which it is).

running that url should work just like clicking on a newsgroup url - if you do
it from the browser, it will bring up the three pane ui, and select the
folder/newsgroup if it exists in the folder pane, and offer to subscribe you to
it if it doesn't. If the folder doesn't exist, we'll still offer to subscribe
you to it (because we don't know if it exists or not), but it will fail when we
try and we'll give you the message from the server explaining the error.

Comment 6

[David :Bienvenu]

when we do this, we're going to run into a problem if the user has two accounts
on the same imap server as the url they're clicking on - we won't know which
account they mean to subscribe to the folder on. This is a rare scenario, so
when this is fixed, I'll try to remember to file a new bug on that scenario, and
relnote it. The user can always use the subscribe UI to subscribe to the folder
on the right account.

Comment 7

[jglick]

So, we need a "Copy Folder Location" menu item in the context menu added? 

For IMAP folders only, correct?
Only when the folder is shared or always?

Comment 8

[David :Bienvenu]

it can be for all folders. The difference is that a url for imap folders might
be useful for other users, but a uri for a local folder is only useful for the
user of the machine containing the folder, if that makes sense.

Comment 9

[jglick]

OK, adding "_C_opy Folder Location" to Mail content menu spec for folders. 
Should appear on mozilla shortly. I added a separator as well (below 
Subscribe and above all the folder action items) since the menu is getting a bit 
full. Example for generic folder shown below.


Open in New Mail Window
Copy Folder Location
Subscribe...
------------------
New Subfolder...
Rename Folder...
Compact This Folder
Delete Folder
-------------------
Search Messages... 
Folder Properties

Does the "Unsent Messages" folder need "Copy Folder Location" as well?

Spec location:
http://www.mozilla.org/mailnews/specs/threepane/MailMenus.html

Comment 10

[scottputterman]

moving to 1.0

Comment 11

[David :Bienvenu]

I have the url link clicking basically working so that it subscribes you to the
other user's folder and selects it.  I'm cleaning up the patch and making sure
it works with folder names that require escaping and imap mod-utf7 encoding.

Comment 12

[David :Bienvenu]

Attachment: add copy folder location to folder pane context menu

this adds the "Copy folder location" menu item

Comment 13

[David :Bienvenu]

Attachment: news diffs to get copy location working for news

this diff gets CopyFolderLocation working for newsgroups.

Comment 14

[David :Bienvenu]

Attachment: fix nsImapMailFolder::GetFolderUrl

make imap folders GetFolderUrl generate a good url

Comment 15

[David :Bienvenu]

Attachment: resource diffs for copy folder location menu item

Comment 16

[David :Bienvenu]

Navin and Seth, can I get reviews for the copy folder location part of this bug?
The non-obsolete patches all need review. I'll attach the mega-patch to make the
clicking on urls sometime soon.

Comment 17

[Navin Gupta]

Comment on attachment 71934 [details] [diff] [review]
news diffs to get copy location working for news

r=naving

Comment 18

[David :Bienvenu]

clicking on a link to an imap folder in the browser is not going to work because
security says No - this is terribly unfortunate, because it means administrators
can't put up web pages that allow users to subscribe to shared imap folders. 
The code in question is in nsScriptSecurityManager::CheckLoadURI. I'm not sure
if this code is trying to distinguish between users clicking on links or generic
url running (in the theory that links can be disguised). I'm a bit confused why
the normal imap password protection is not sufficient here - if the user doesn't
know the password, I'm not sure what harm clicking on the link is going to do.
Cc'ing Mitch for some words of wisdom here.

Comment 19

[(not reading, please use [e-mail] instead)]

Comment on attachment 71934 [details] [diff] [review]
news diffs to get copy location working for news

sr=sspitzer

Comment 20

[(not reading, please use [e-mail] instead)]

Comment on attachment 71936 [details] [diff] [review]
fix nsImapMailFolder::GetFolderUrl

sr=sspitzer

in your comment about imap URIs can you refer to bug #84045, and then in the
bug, refer back to this code?

one day, we'll clean this up.

Comment 21

[Navin Gupta]

Wait a minute, can you explain why imap folder url generation is different
from news. I see for news you are getting host, port etc but not for imap. 

Comment 22

[David :Bienvenu]

imap urls are of the form: root folder url + foldername, escaped.  Perhaps news
folders urls could be constructed the same way; I don't know, but that's not
really your question.

imap urls are of the form imap://user@host/foldername - they're different from
news. (there's an IMAP URL standard, btw, so we don't have any choice in the
matter).

Comment 23

[Navin Gupta]

Comment on attachment 71936 [details] [diff] [review]
fix nsImapMailFolder::GetFolderUrl

r=naving

Comment 24

[David :Bienvenu]

Attachment: use GetSelectedFolderResource per Seth

this doesn't obsolete the other res changes, just the changes to
mailContextMenu.js

Comment 25

[David :Bienvenu]

Attachment: just diffs for mailContextMenu.js

Comment 26

[(not reading, please use [e-mail] instead)]

Comment on attachment 71973 [details] [diff] [review]
just diffs for mailContextMenu.js

sr=sspitzer

Comment 27

[(not reading, please use [e-mail] instead)]

Comment on attachment 71938 [details] [diff] [review]
resource diffs for copy folder location menu item

sr=sspitzer, for the .xul and .dtd (js covered by a more recent patch.)

Comment 28

[Navin Gupta]

r=naving on FE changes

Comment 29

[David :Bienvenu]

Attachment: proposed fix

this fix makes it so that news links and imap links work from a mail message or
a browser window (modulo the security crippling of imap urls run from web
pages).  It's a lot of code, but almost none of it is run unless the user
clicks on an external link. I added an attribute to imap urls that says whether
they're external or not - any url run from mailnews code will go through the
imap service and set the url to internal. 
The basic strategy is when run from a mail window, just run the url in the mail
window, but if run when there's no mail window, abort the url and open a new 3
pane window and run the url there. I had to do a lot of escaping and unescaping
and fiddling with urls to get the url to match the expectations so that we can
select the folder when done.
For the case where we have to subscribe to an imap folder and then select it, I
had to detect this case in nsImapMailFolder::OnStopRunningUrl.

Comment 30

[David :Bienvenu]

requesting reviews. I also had to change the way we launch the 3 pane and open a
folder and/or msg (the mailnews/base changes) and with corresponding changes to
news.

Comment 31

[David :Bienvenu]

Navin, any chance of getting a review?

Comment 32

[(not reading, please use [e-mail] instead)]

looks good.

see my comments about casting away the constness of nsXPIDLCString buffers.  
other than that, it's all nits.  too bad about #84045 and our broken imap uri 
story.  that would have saved you some grief.

1)  can you comment out this dump before checking in?

+    dump("start folder uri = " + gStartFolderUri + "key = " + gStartMsgKey);

2)
 
   nsCOMPtr<nsIWindowWatcher> wwatch(do_GetService
("@mozilla.org/embedcomp/window-watcher;1"));
+  nsresult rv;
+  if (!wwatch)
     return NS_ERROR_FAILURE;

instead, do:

nsCOMPtr<nsIWindowWatcher> wwatch = do_GetService
("@mozilla.org/embedcomp/window-watcher;1", &rv);
NS_ENSURE_SUCCESS(rv,rv);

3)

you might not need "nsXPIDLCString args" in OpenMessengerWindowWithUri()

maybe something like this would this do the trick?

+  rv = openWindow(NS_ConvertASCIItoUCS2(chromeurl).get(), uri ? 
NS_ConvertASCIItoUCS2(uri).get() : nsnull, nsMsgKey_None);

4)

I'm not sure, but I think you can just do %S here.  (doesn't really matter)

+
+# LOCALIZATION NOTE (autoSubscribeText): %1$S is the imap folder.
+5092=Would you like to subscribe to %1$S?

5)

thanks for renaming aWindow to msgWindow (since it wasn't an arg.)

while you are here, spelling fix:  proably -> probably
 
-    if (!aWindow) // if we don't have a window then we are proably running a 
biff url
+    if (!msgWindow) // if we don't have a window then we are proably running a 
biff url

6)

+
+    if ((const char *) userName)
+      nsUnescape(NS_CONST_CAST(char*,(const char*)userName));

This is scary.  You are casting away the constness to the buffer on an 
xpidlcstring, so
that you can unescape the buffer.  this makes me nervous, I'm not sure you are 
guaranteed
to be allowed to poke the buffer like this.  (for example, this depends on the 
string implementation.

unescaped strings will take less space than escaped strings, but will .Length() 
still work on the xpidlcstring?

I think it would be best, in this scenario, to strdup username, and unescape 
that.

7)  just the normal nit of do:

if (NS_FAILED(rv))
  return rv;

to aid debugging.

+    rv = mailnewsUrl->GetFileName(getter_Copies(folderName));
+    if (NS_FAILED(rv)) return rv;
...  
+    nsCOMPtr<nsIMsgAccountManager> accountManager = 
+             do_GetService(NS_MSGACCOUNTMANAGER_CONTRACTID, &rv);
+    if (NS_FAILED(rv)) return rv;
...
+    // if we can't extract the imap server from this url then give up!!!
+    if (NS_FAILED(rv)) return rv;
+    NS_ENSURE_TRUE(*aServer, NS_ERROR_FAILURE);
+    return rv;

8)  same concern as above, casting away the constness of the xpidlcstring 
buffer.

+      nsXPIDLCString folderName;
+      imapUrl->CreateCanonicalSourceFolderPathString(getter_Copies
(folderName));
+      if (folderName.IsEmpty())
+      {
+        rv = mailnewsUrl->GetFileName(getter_Copies(folderName));
+        if (!folderName.IsEmpty())
+          nsUnescape(NS_CONST_CAST(char*,(const char*)folderName));
+      }

9)


+    if (NS_SUCCEEDED(rv))
     *_retval = mockChannel;
     NS_IF_ADDREF(*_retval);

this be:
 

     if (NS_SUCCEEDED(rv))
       NS_IF_ADDREF(*_retval = mockChannel); 

10)

 
+  if (nsCRT::strcasecmp(aContentType, "x-application-imapfolder") == 0)
+  {

curious, why strcasecmp, instead of strcmp().  I don't see this anywhere else 
with caps, or am I missing something?

11)

+      // imap uri's are unescaped, so unescape the url.
+      nsUnescape(NS_CONST_CAST(char*,(const char*)uriStr.get()));
+      nsCOMPtr <nsIMessengerWindowService> messengerWindowService = 
do_GetService(NS_MESSENGERWINDOWSERVICE_CONTRACTID,&rv);
+      NS_ENSURE_SUCCESS(rv, rv);
+
+      rv = messengerWindowService->OpenMessengerWindowWithUri("mail:3pane", 
uriStr.get());
+      NS_ENSURE_SUCCESS(rv, rv);

again, casting away constness.

12)  

why was this assertion commented out?  if bogus, we should remove it.

-	NS_ASSERTION(newMsgHdr, "CreateNewHdr didn't fail, but it returned a 
null newMsgHdr");
+//	NS_ASSERTION(newMsgHdr, "CreateNewHdr didn't fail, but it returned a 
null newMsgHdr");
 	if (!newMsgHdr) return NS_ERROR_NULL_POINTER;

Comment 33

[David :Bienvenu]

Seth, thanks for reviewing this. I didn't add the casting away of the constness
for the user name - I just moved it into another function, and copied the
technique for the uri's. I agree that it's scary, and I wouldn't have done it if
we weren't doing it in a bunch of other places. I can change it.

I use nsCRT::strcasecmp for the content type because all the other code checking
the content type uses a case insensitive compare - I think content types are
supposed to be case-insensitive, though you are right, our code only generates
the lower case content type.

I won't check in the assertion change - I don't remember why I commented that
out and didn't mean it to be part of the diff.

Comment 34

[Navin Gupta]

Comment on attachment 72181 [details] [diff] [review]
proposed fix

r=naving

could use strdup

+    if (imapAction == nsIImapUrl::nsImapSelectFolder)
+      *aContentType = nsCRT::strdup("x-application-imapfolder");

Add back this assertion
-	NS_ASSERTION(newMsgHdr, "CreateNewHdr didn't fail, but it returned a 
null newMsgHdr");
+//	NS_ASSERTION(newMsgHdr, "CreateNewHdr didn't fail, but it returned a 
null newMsgHdr");

Comment 35

[David :Bienvenu]

I think I'll stick with nsCRT::strdup - too many people have broken the build by
changing nsCRT::strdup to strdup (don't ask me why, but you won't find many
instances of just plain strdup in our code).

Comment 36

[(not reading, please use [e-mail] instead)]

Comment on attachment 72181 [details] [diff] [review]
proposed fix

sr=sspitzer, bienvenu says he make the necessary changes locally, before he
checks in, and that's good enough for me.

Comment 37

[Asa Dotzler [:asa]]

Comment on attachment 72181 [details] [diff] [review]
proposed fix

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 38

[David :Bienvenu]

fix checked in - imap urls won't work from a web page because of caps won't
allow it for now but they should work from a mail message.

Comment 39

[Karen Huang]

Used 05-22-08-1.0.0 build
Yes. It's working from the mail message, but it doesn't work from the Browser 
URL. Is this related to other Browser caps bug? Can you put that browser bug for 
this bug's dependance? I think I should verify this bug on the Browser URL as 
well......

Comment 40

[Karen Huang]

Attachment: attach the confirm dialog that displaying when subscribed the shared folder from the mail message.

Comment 41

[Karen Huang]

From David's email:
-------------------------------------------------------------------------------
The security/caps code deliberately, expressly, on purpose, does not allow imap 
urls to run
from the browser. I don't know if there's a bug about it, but if there was, I'm 
pretty sure they'd mark
it WONTFIX or INVALID because they don't want to allow imap urls to work from 
the browser.
--------------------------------------------------------------------------------

OK. Based on David's email, I am marking as verified for now.

Comment 42

[Karen Huang]

I don't know whether I need to reopen this bug or not since it seems that the 
current branch build performs differently as before:

If I Subscribe the IMAP Shared folders for NMS 4.15 p7 (parp.mcom.com):
From the Browser URL -> it works.
From the Mail Message -> it doesn't work.

If I Subscribe the IMAP Shared folders for NMS 6.1 (tintin.mcom.com):
From the Browser URL -> it does not work and display an incorrect IMAP error 
(see bug 157621)
From the Mail Message -> it doesn't work either. :-(

I remembered this used to work at least from Mail Message... is this regression?
Also, is bug 127702 fix related to this problem?

Comment 43

[Karen Huang]

IMAP URLs that I used for subscribing huang1 account for huang/ReadPrivilege 
folder:
imap://user@host/foldername
(e.g. imap://[e-mail]/ReadPrivilege)
imap://userid@host/Shared%20Folders/User/userid/shared_fdr_name
(e.g. imap://[e-mail]/Shared%20Folders/User/huang/ReadPrivilege)

I am attaching the screen shots for the URL results of Mail Messages as 
following:

Comment 44

[Karen Huang]

Attachment: screen shot for the result after selecting URL from the Mail message

Comment 45

[David :Bienvenu]

Karen, why don't you forward me the actual message you tried this with, instead
of retyping the urls in the bug report? And you might also send me your prefs.js
so I can look at your accounts.

Comment 46

[Karen Huang]

It seems that I can subscribe shared folders from the browser IMAP URLs for NMS
6.1 & 4.15 Servers...so do we change to support IMAP subscribe shared folders on
the broswer URL already? (no concerns for Security issues anymore??)

Yes! The results are same as David described: 
After I paste an imap url directly into a mail message, the url-ification code
converts them to mailto urls, unfortunately.
But, if I create a link, e.g., "click here to subscribe" and set the link url to
the imap url, shared folders will get subscribed correctly......

Comment 47

[David :Bienvenu]

So, if I understand correctly, the bug is that when you send a mail message, raw
imap urls get converted to mailto: urls because of the user@server part of the
url? That's a different bug, and this bug should be reclosed as fixed? I'm not
sure what you mean about the security problem being not valid anymore.

Comment 48

[Karen Huang]

Based on previous David's comment, resolved this bug as fixed again.

Comment 49

[Karen Huang]

I have logged bug #158016 for tracking above "imap urls get converted to mailto: 
urls" problem.
I also logged bug 158023 & bug 158046 based on David's Comment#5 and Comment #6 
suggestion.

Since this does allow imap urls to run from the browser for the 
current branch build. I am just curious about the Security issue that David 
mentioned in Comment #41.
Anyway, I am glad this also works from the browser and marking as verified for 
this bug now.

Windows 07-17-08-1.0 
Linux 07-17-07-1.0
Mac 07-17-05-1.0