133823 - Composer cannot edit FTP very well unless username is included in document URL

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect)

Description

[michael wendell]

This was originally filed as part of bug 133083.

When publishing images to another directory, I am getting a "user anonymous
unknown " message, even though both the image and the HTML file are uploaded. 
The only other thing is that the url of the image is changed on the HTML file. 
But that issue is covered in 133083.

Steps to Reproduce:
1. Restart NS and launch Composer
2. Create a simple page with two lines of text and one image
3. Click on File - Publish As
4. Fill in all the appropriate information for your FTP server
5. Make sure to check the boxes to "Include images and other files" and "Use
this site subdirectory" Type the name of an image directory
6. Click Publish

Actual Results:
I recieve a message "user anonymous unknown", even though both the HTML file and
the image are uploaded.

Expected Result:
I would expect that I would not get a message since the upload(s) went through fine.

Comment 1

[Charles Manske]

It has now become apparent that Composer is totally screwed by the FTP behavior
of assuming anonymous login if username+password is missing or fails.
We simply cannot edit FTP urls directly with this problem.
In the Publish dialog (Settings Panel) or Publish Settings dialog, the user can
supply a HTTP address that is the alias for the FTP address. If that is not
supplied, we get "login failed" when publishing files (see bug 133823)
This is because after publishing, we change the URL of the page to the ftp
address, but without the username+password, and BOTH are needed to successfully
load the page. We could insert the "username:password@" into the document URI,
but I think that is very evil. You NEVER want the password to show in plain text,
of course, like in the window title and many other places that access the 
document's location.href. So after publishing like this, further attempts to
save changes will result in constant "login failed" alerts.

So publishing via FTP *really* requires a correct HTTP alias address. You should
never be editing an FTP url directly.

Comment 2

[Charles Manske]

Luckily, it turns out that this is mostly caused by bug 134248. The document
URL is mangled by StripUsernamePassword (scheme is deleted!), which is causing
the "Login incorrect" messages.

Comment 3

[Charles Manske]

I'm finally comming to grips with the intracacies of FTP publishing.
To make this all work, we must always include the username in the final 
document URL when editing a page with an FTP address, thus after publishing with
FTP, the document URL should be somthing like:
ftp://[e-mail]/myfile.html
If we do that, when the user edits a page, she *will* be prompted for a password,
instead of getting the insidious "Login incorrect" alert dialog. And if we 
store the password correctly in the publishing code, user will not be prompted
at all.
This has consequences in various parts of the publishing code. E.g.:
1. When trying to find existing Publishing sites in the site database by
matching the document URL, we must pay attention to the username (this fixes the
problem of supporting multiple FTP users at a site, bug 130737)
2. Password saving and retrieval must be tweaked as the url used is sensitive
to whether or not the username is embedded in the url. In addition, password
manager urls do NOT include the terminal "/" in its site urls.
3. Realizing that users may enter "fully qualified" urls in the browser to view
an FTP page (e.g.: ftp://username:[e-mail]/myfile.html), and then
use "Edit Page" to edit it in Composer, we have to be very careful to not expose
the password in plain text in various places in Composer, such as in the 
Recent Files menu and in Page Properties. And we certainly should strip out
the password when storing a url in recent files history prefs.

Comment 4

[Charles Manske]

*** Bug 130737 has been marked as a duplicate of this bug. ***

Comment 5

[Charles Manske]

Attachment: Patch v1

This fixes all the problems described above.
In composer commands:@@ -1195,7 +1203,10 @@: Updates changed password in
publish data after getting
a password prompt from FTP.

@@ -1556,20 +1581,25 @@ is in the method GetDocUrlFromPublishData() that 
figures out the document url from the publishing data

Changes to editor.js are for the "Recent Files" menu.

Changes to publishprefs.js fixes all problems with password handling.
It's more changes then I'd like, but to do what was needed was making things
too complicated, so instead I replaced the  very funky method 
"GetMatchingPublishUrlLength()" with the much more understandable 
"FillInMatchingPublishData()" [Brade will really like that :)]
This simplifies and cleans up code relating to finding publish data saved 
in prefs given a document url.

Comment 6

[Bradley Baetz (:bbaetz)]

Comment on attachment 76918 [details] [diff] [review]
Patch v1

>Index: ComposerCommands.js
>===================================================================
>RCS file: /cvsroot/mozilla/editor/ui/composer/content/ComposerCommands.js,v
>retrieving revision 1.113
>diff -u -r1.113 ComposerCommands.js
>--- ComposerCommands.js	27 Mar 2002 03:45:49 -0000	1.113
>+++ ComposerCommands.js	31 Mar 2002 00:44:45 -0000

>+      if (!ret)
>+        setTimeout("CancelPublishing()",0);

do: |setTimeout(CancelPublishing, 0);| instead

I didn't see anything else on a quick look trhough, but I'm not likely to have
time to apply and test until the middle of next week.

Comment 7

[Kathleen :Brade]

I need to discuss this patch with Charley.  I don't see where
"InsertUsernameIntoUrl" is.  Also, I don't think the issue of showing people's
passwords in various places should be buried in this bug (since it's such an
important issue).

Comment 8

[Charles Manske]

InsertUsernameIntoUrl() was already checked in.
Changes for "setTimeout" were done as suggested.

Comment 9

[Kathleen :Brade]

Comment on attachment 76918 [details] [diff] [review]
Patch v1

r=brade for the changes in the patch in ComposerCommands.js (with bbaetz'
suggestion) and publishprefs.js

These should be in a separate bug to track the issue:
 * EdPageProps.js
 * editor.js
I worry that these changes are incomplete

Comment 10

[Charles Manske]

The "strip password from urls" issue is now in bug 134695, so please ignore
changes to editor.js and EdPageProps.js in "Patch v1"

Comment 11

[Charles Manske]

This bug is critical to Publishing feature

Comment 12

[kinmoz]

-      if (ret && gPublishData)
+      if (!ret)
+        setTimeout("CancelPublishing()",0);
+
+      if (gPublishData)
         UpdateUsernamePasswordFromPrompt(gPublishData, gPublishData.username, 
pwObj.value, saveCheck.value);


In the old code, you only executed UpdateUsernamePasswordFromPrompt() when |ret| 
was non-zero, now you don't check it before calling it. Intentional?

FindSiteIndexAndDocDir() now calls FillInMatchingPublishData() in a loop, is 
there a potential that the list we are looking through is long? I ask because 
the passed in docURL won't change between each iteration of the loop yet, each 
call to FillInMatchingPublishData() will do the same work to split the docURL 
into pieces and strip out the password. I'm just wondering if it's more 
efficient to restructure some of the code to do this work once?

Comment 13

[Charles Manske]

Kin: To be completely correct, yes the new line should be:
+      if (ret && gPublishData)
         UpdateUsernamePasswordFromPrompt(gPublishData, gPublishData.username, 
pwObj.value, saveCheck.value);

although it doesn't really matter since ret = false means user canceled 
publishing and we will delete gPublishData in CancelPublish(), so the effect of
UpdateUsernamePasswordFromPrompt is ignored. 
I'll make that change.

Comment 14

[Charles Manske]

On the issue of:
"FindSiteIndexAndDocDir() now calls FillInMatchingPublishData() in a loop"
Yes, this is less efficient. No, I don't expect these lists to be very long;
it's the number of successfully-published directories within a publishing site.
IMHO, the method FillInMatchingPublishData() is so much better than the obtuse
GetMatchingPublishUrlLength() that the benefit of that clarity outways the 
performance hit.
Brade: Don't you agree?

Comment 15

[Charles Manske]

Oops! I was wrong in last comment: The number items in list (number of times 
FillInMatchingPublishData() is called) is the number of publishing sites in the
prefs, not the number of subdirectories. I'd expect the former to be even less
than the latter, since the code tries very hard to keep the number of sites down
by using a separate list of subdirectoriest for each site.

Comment 16

[kinmoz]

Comment on attachment 76918 [details] [diff] [review]
Patch v1

sr=[e-mail] with the |ret| change mentioned above, and with the
assumption that as you suggest there is no significant perf penalty to worry
about.

Comment 17

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on behalf of ADT) for checkin to the 1.0 trunk.

Comment 18

[michael wendell]

As of the 04-02 trunk build, I am not recieving the original "user anonymous
unknown" message.  

But it looks like this patch will fix much more than just the original problem.

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 76918 [details] [diff] [review]
Patch v1

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

Comment 20

[Charles Manske]

checked in

Comment 21

[michael wendell]

Verified on 04-05 trunk.

Comment 22

[Charles Manske]

*** Bug 136430 has been marked as a duplicate of this bug. ***

Comment 23

[Charles Manske]

*** Bug 136431 has been marked as a duplicate of this bug. ***