Last Comment Bug 395654 - failure to renegotiate digest authentication
: failure to renegotiate digest authentication
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: Provider: CalDAV (show other bugs)
: unspecified
: All All
: -- normal (vote)
: 0.8
Assigned To: Bruno Browning
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-09-10 04:08 PDT by Bruno Browning
Modified: 2008-02-26 02:00 PST (History)
1 user (show)
dbo.moz: wanted‑calendar0.8+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
don't assume existing authentication on refresh (4.82 KB, patch)
2007-10-03 15:28 PDT, Bruno Browning
cmtalbert: review+
Details | Diff | Splinter Review
Un-bitrotted version of patch (4.83 KB, patch)
2007-11-26 20:44 PST, cmtalbert
cmtalbert: review+
Details | Diff | Splinter Review
queue refresh()es of calendars sharing authrealm (18.39 KB, patch)
2008-02-14 21:43 PST, Bruno Browning
cmtalbert: review-
Details | Diff | Splinter Review
corrections per reviewer (19.92 KB, patch)
2008-02-21 19:01 PST, Bruno Browning
cmtalbert: review+
Details | Diff | Splinter Review

Description Bruno Browning 2007-09-10 04:08:14 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.6) Gecko/20061201 Firefox/2.0.0.6 (Ubuntu-feisty)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.7pre) Gecko/20070909 Calendar/0.7pre

Servers providing digest authentication often require periodic re-authentication. Sunbird does fine with initial authentication, but when later presented with a 401 response it prompts the user for name/password instead of re-authenticating using cached credentials.

Reproducible: Always

Steps to Reproduce:
1. Create and log in to a remote calendar using digest authentication. An Apple CalendarServer in the default configuration should do nicely
2. Set the refresh time to something reasonably short, like one minute, just to speed up testing
3. Wait until the server requests re-authentication. With the Apple server, this seems to be about 15 minutes
4. Observe that next refresh() causes a password prompt to appear (actually, several of them appear, but that's kind of to be expected if the first one does. This would probably happen on add/modify/delete as well as refresh; not tested)
Actual Results:  
User is prompted for name/password

Expected Results:  
User should not be re-prompted for credentials while Sunbird is running. I would expect Sunbird to respond to the second 401 from the server by re-authenticating using cached credentials.

Having the password manager remember passwords does not help.

Nominating this as a 0.7 blocker, as it makes Sunbird too annoying to use with CalendarServer, which will be released in our 0.7 timeframe, at least in CS's default configuration.
Comment 1 Daniel Boelzle [:dbo] 2007-09-10 04:36:48 PDT
Although this is annoying w.r.t CalendarServer, it IMO doesn't slip 0.7, but is nice to have.
Comment 2 Michiel van Leeuwen (email: mvl+moz@) 2007-09-10 09:51:59 PDT
sunbird itself doesn't have much to do with authentication. I suspect that this is a necko problem. What happens when you use firefox to access such a server?
And does it make any difference if you use lightning? And do trunk builds show the problem?
Comment 3 Bruno Browning 2007-09-27 18:57:10 PDT
Problem was reproducible with a variety of browsers on Linux/Windows/Mac (Lightning does show this problem; unable to test trunk build because auth is pretty well broken there). Wireshark showed that CalendarServer was timing out the authentication and returning a 401 challenge with a new nonce value but not a 'stale="true"' assertion. Notified the CalendarServer chaps, they fixed the CalendarServer bug, and now Firefox etc properly re-authenticate w/o prompting. 

But Calendar still re-prompts authentication instead of renegotiating it with the original credentials.
Comment 4 Bruno Browning 2007-09-30 19:21:40 PDT
With the CalendarServer bug fixed, this mozilla bug is fixed IF the first action Sb/Ltn takes after the server times out the auth is is a PUT, or a view change. We still have this issue wrt refresh, though. Seems to be bug 335462 come back to haunt us: 'refresh' causes several simultaneous http requests, and if the server resource being targeted has become unauthenticated necko cant't re-auth all of them simultaneously, thus multiple auth prompts.

Resetting component as this is now a CalDAV-specific bug.
Comment 5 Bruno Browning 2007-10-03 15:28:48 PDT
Created attachment 283450 [details] [diff] [review]
don't assume existing authentication on refresh

While I'd love to see this in 0.7, it really ought to be tested more widely than just by me, so I'm thinking it's too late to make the cut. It's a bug that's liable to be seen by a fair number of people once CalendarServer is released, but workarounds (don't use digest auth, don't refresh) exist.
Comment 6 cmtalbert 2007-11-26 20:43:28 PST
Comment on attachment 283450 [details] [diff] [review]
don't assume existing authentication on refresh

This looks good.  I didn't have an Apple CalDav server to test with, but on testing with other calDav servers it works OK.  This current patch doesn't apply to the current tree, will post back with an unbitrotted version and will carry my review forward.
Comment 7 cmtalbert 2007-11-26 20:44:35 PST
Created attachment 290334 [details] [diff] [review]
Un-bitrotted version of patch
Comment 8 Bruno Browning 2007-12-01 21:06:01 PST
delaying commit because it's looking like either Philipp or I will fix this another way before 0.8.
Comment 9 Philipp Kewisch [:Fallen] 2007-12-02 01:48:43 PST
Do you mean me? I'm not quite sure how I would solve this?
Comment 10 Bruno Browning 2007-12-02 04:49:09 PST
(In reply to comment #9)
> Do you mean me? I'm not quite sure how I would solve this?
> 

The underlying problem here is not that necko fails to renegotiate the auth, it's that necko renegotiates one request at a time, and under certain circumstances (like refresh() ) we issue several requests simultaneously, and necko trips over its own feet trying to renegotiate several requests at the same time. In bug #400283 comment #1 you implied that offline mode / caching would be done in such a way that those multiple requests at refresh() would be consolidated into a single one, which I expect would fix this bug as a side effect. If we also get down to a single consolidated request at startup time, I could rip out a bunch of tiresome request queueing code, not to mention the perf win of not refetching the same data for e.g. both the view and the unifinder.

Comment 11 Bruno Browning 2008-02-14 21:43:23 PST
Created attachment 303463 [details] [diff] [review]
queue refresh()es of calendars sharing authrealm

This bug was partially fixed by the checkin in bug 393817, with which CalDAV calendars will no longer trip over themselves at refresh(). They will, however, still trip over other calendars in the same authentication realm. So if you have only one calendar on a given CalDAV server you're fine; if you have more than one this bug is back. This patch queues refresh()es of all calendars in any given (Digest-schemed) authrealm.

Not sure that the patch is well-formed; my cvsdo util seems to be misbehaving.

It would be nice to see this in 0.8. I realize it's probably a bit late for that, though.
Comment 12 cmtalbert 2008-02-18 21:02:36 PST
Comment on attachment 303463 [details] [diff] [review]
queue refresh()es of calendars sharing authrealm

Bruno, in general this looks good, I have a couple of comments that I'm going to have to - for.
Sorry.  Details below...

>? calendar/providers/caldav/public
>Index: calendar/providers/caldav/calDavCalendar.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/providers/caldav/calDavCalendar.js,v
>retrieving revision 1.45.2.67
>diff -p -8 -u -r1.45.2.67 calDavCalendar.js
>--- calendar/providers/caldav/calDavCalendar.js	13 Feb 2008 13:45:02 -0000	1.45.2.67
>+++ calendar/providers/caldav/calDavCalendar.js	15 Feb 2008 05:00:06 -0000
>@@ -65,25 +65,28 @@ function calDavCalendar() {
> function getLocationPath(item) {
>@@ -119,17 +122,18 @@ calDavCalendar.prototype = {
>     __proto__: calProviderBase.prototype,
>     //
>     // nsISupports interface
>     //
>     QueryInterface: function (aIID) {
>         return doQueryInterface(this, calDavCalendar.prototype, aIID,
>                                 [Components.interfaces.calICalendarProvider,
>                                  Components.interfaces.nsIInterfaceRequestor,
>-                                 Components.interfaces.calIFreeBusyProvider]);
>+                                 Components.interfaces.calIFreeBusyProvider,
>+                                 calICalDavCalendar]);
>     },
>+
>+    get authRealm() {
>+        return this.mAuthRealm;
>+    },
>+
>+    set authRealm() {
>+        // this is done at startup
>+    },
If startup doesn't need this function to set the authRealm, can we make authRealm a readonly attribute?  If not, then why doesn't startup use this function to set the authRealm instead of going around the new interface?

>@@ -1221,23 +1288,74 @@ calDavCalendar.prototype = {
>+            var resourceTypeXml = multistatus..D::["resourcetype"];
>+            if (resourceTypeXml.length == 0) {
>+                resourceType = kDavResourceTypeNone;
>+            } else if (resourceTypeXml.toString().indexOf("calendar") != -1) {
>+                resourceType = kDavResourceTypeCalendar;
>+            } else if (resourceTypeXml.i.toString().ndexOf("collection") != -1) {
>+                resourceType = kDavResourceTypeCollection;
>+            }
This last else if looks incorrect. Is it supposed to be resourceTypeXml.toString().indexOf?

>+++ ./calICalDavCalendar.idl
>@@ -0,0 +1,58 @@
>+/* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Mozilla calendar code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Bruno Browning <browning@uwalumni.com>
>+ * Portions created by the Initial Developer are Copyright (C) 2008
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ *
Forgot the Contributor(s) bit here.  Check: http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c (it's also missing on the Makefile.in too)

>+/** Adds CalDAV specific capabilities to calICalendar.
>+ */
>+[scriptable, uuid(88F6FB22-C172-11DC-A8D1-00197EA74E11)]
>+interface calICalDavCalendar : calICalendar
>+{
>+    /**
>+     * The calendar's RFC 2617 authentication realm
>+     */
>+    attribute AUTF8String authRealm;
Again, if we aren't going to need to use "setAuthRealm()" then let's make this a readonly attribute.  Otherwise, let's use setAuthRealm() as I mentioned above.
Comment 13 Bruno Browning 2008-02-21 19:01:07 PST
Created attachment 304882 [details] [diff] [review]
corrections per reviewer

Thanks, Clint. I think this version addresses your concerns; it also adds the new .xpt file to the windows installer, which I forgot the first time around.
Comment 14 cmtalbert 2008-02-22 12:44:28 PST
Comment on attachment 304882 [details] [diff] [review]
corrections per reviewer

This looks good, Bruno.  Thanks!  r=ctalbert
Comment 15 Bruno Browning 2008-02-23 15:38:05 PST
patch checked in on trunk and MOZILLA_1_8_BRANCH

->FIXED

Note You need to log in before you can comment on or make changes to this bug.