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.

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"

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

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.

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?

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.

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.

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?

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.

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

moving to 1.0

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.

this adds the "Copy folder location" menu item

this diff gets CopyFolderLocation working for newsgroups.

make imap folders GetFolderUrl generate a good url

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 on attachment 71934 [details] [diff] [review] news diffs to get copy location working for news r=naving

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 on attachment 71934 [details] [diff] [review] news diffs to get copy location working for news sr=sspitzer

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.

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.

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 on attachment 71936 [details] [diff] [review] fix nsImapMailFolder::GetFolderUrl r=naving

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

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

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

r=naving on FE changes

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.

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.

Navin, any chance of getting a review?

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;

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

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 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 on attachment 72181 [details] [diff] [review] proposed fix a=asa (on behalf of drivers) for checkin to the 1.0 trunk

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.

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

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.

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?

IMAP URLs that I used for subscribing huang1 account for huang/ReadPrivilege folder: imap://user@host/foldername (e.g. imap://huang@tintin.mcom.com/ReadPrivilege) imap://userid@host/Shared%20Folders/User/userid/shared_fdr_name (e.g. imap://huang@tintin.mcom.com/Shared%20Folders/User/huang/ReadPrivilege) I am attaching the screen shots for the URL results of Mail Messages as following:

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.

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

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.

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

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