335462 - CalDAV provider prompts multiple times for already-provided auth info

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[Bruno Browning]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.13) Gecko/20060418 Fedora/1.0.8-1.1.fc4 Firefox/1.0.8
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060425 Mozilla
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060425 Mozilla Sunbird/0.3a1+

Sunbird has begun prompting five times for name/password when the calendar is defined with authentication information embedded in the URI, e.g. http://name:pass@host...

Reproducible: Always

Steps to Reproduce:
1. Define a calDAV calendar in Sunbird with auth info in the URI
2. Enter name, password in dialog box
3. Repeat 2. as needed

Actual Results:  
Sunbird prompts for auth info five times, then allows access to calendar

Expected Results:  
Sunbird should allow access to calendar without asking for info embedded in the URI

Seems to have happened right after the recent fixes to 308567. No good deed...

Comment 1

[Zach Beauvais]

This occurs also in Lightening 0.1 running in TBird 1.5. I have found that it can request more than five times, however... upwards of 20, so far. It will occur when any publishable change is made... whenever a task is deleted, for example. I have to say, it has rather put me off using the thing, as it has wasted a lot of time, and completely renders the tasks useless. 

Comment 2

[Sebastian Schwieger]

Confirming using Cosmo-Server 0.4 and Sunbird 2006082207

Comment 3

[cmtalbert]

Per Comment#2, confirming. I've seen this on windows as well.

Comment 4

[Celina]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060821
Calendar/0.3a2+

Bug is still occurring in 20068021 with the following changes:
Upon first subscribing to remote calendar, 3 password prompts (all fully
visible, not just titlebar)
Upon Calendar close/relaunch, 5 password prompts (all fully visible).

Number of tasks/events does not appear to effect the number of prompts.  One
thing noticed is after the 2nd prompt on first subscribing, and 4th prompt on
re-launch, tasks/events are loaded and focus is taken from the password prompt.

Comment 5

[Bruno Browning]

Attachment: queue requests pending authentication

Problem seems to be that Sunbird's initial flurry of SELECTs translates on CalDAV to a flurry of HTTP requests against an as-yet-unauthenticated resource. This patch queues the second and subsequent requests in that flurry, and only issues them after the first has returned successfully and auth info is available.

There will be a similar issue wrt the ICS provider in the use-case of multiple .ics calendars on a single WebDAV server (and thus auth realm), which is most likely at least part of what's going in in bug 248903 and possibly bug 343721.

Comment 6

[Matthew (lilmatt) Willis]

I've tested this patch successfully against Apple's CalDAV server (current svn HEAD).

I'm not sure if we want to use an AUTF8String for holding status (instead of an int with some constants or something), but I'll leave that to jminta and dmose to decide.

Comment 7

[Bruno Browning]

Attachment: use ints not strings

rev2. use ints not strings, rename array, check array length

Comment 8

[Matthew (lilmatt) Willis]

Comment on attachment 236909 [details] [diff] [review]
use ints not strings

Will replace with new attachment

Comment 9

[Matthew (lilmatt) Willis]

Attachment: updated diff with lilmatt's review comments

r1=lilmatt (applied comments myself due to time constraints at calconnect interop)

Comment 10

[Matthew (lilmatt) Willis]

Comment on attachment 236909 [details] [diff] [review]
use ints not strings

>Index: calendar/providers/caldav/calDavCalendar.js
>===================================================================
>+    // attribute PRUInt8 mAuthenticationStatus;
>+    // 0: no authentication
>+    // 1: first HTTP request sent, subsequent requests going into queue
>+    // 2: freshly authenticated; need to process request queue
>+    // 3: authenticated; startup request queue being processed or empty
>+    mAuthenticationStatus: 0,
When I suggested using ints, I neglected to mention that we should set them to constants for readability. I'm sorry. 

>+    get authenticationStatus() {
>+        return this.mAuthenticationStatus;
>+    },
>+    set authenticationStatus(aStatus) {
>+        this.mAuthenticationStatus = aStatus;
>+    },

Since we don't attempt to use authenticationStatus from outside this file, we don't need the getter and setter. We can just use this.mAuthenticationStatus instead.


These comments were applied and the result is attachment 240194 [details] [diff] [review].

Comment 11

[Matthew (lilmatt) Willis]

Comment on attachment 240194 [details] [diff] [review]
updated diff with lilmatt's review comments

>Index: calendar/providers/caldav/calDavCalendar.js
>===================================================================
>+    // attribute PRUInt8 mAuthenticationStatus;
>+    const kCaldavNoAuthentication = 0;
>+    const kCaldavFirstRequestSent = 1;      // Queueing subsequent requests
>+    const kCaldavFreshlyAuthenticated = 2;  // Need to process queue
>+    const kCaldavAuthenticated = 3;         // Queue being processed or empty
Put these above the prototype.


>+    pendingStartupRequests: Array(),
This should be mPendingStartupRequests since it's a member variable.

>+            // We have a result, so we must be authenticated
>+            if (thisCalendar.authenticationStatus == kCaldavFirstRequestSent) {
>+                thisCalendar.authenticationStatus = kCaldavFreshlyAuthenticated;
>+            }
>+
>+            if (thisCalendar.authenticationStatus == kCaldavFreshlyAuthenticated) {
>+                thisCalendar.authenticationStatus = kCaldavAuthenticated;
This should be mAuthenticationStatus

>+                while (thisCalendar.pendingStartupRequests.length > 0) {
>+                    var req = thisCalendar.pendingStartupRequests.pop();
>+                    thisCalendar.getItems(req[0], req[1], req[2], req[3], req[4]);
>+                }
Use for in instead of while.

r=dmose (verbal) with that

Comment 12

[Matthew (lilmatt) Willis]

Patch (with nits) checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED