Closed Bug 1301422 Opened 8 years ago Closed 7 years ago

Google calendar authentication not remembered anymore / prompts for access on each Thunderbird startup

Categories

(Calendar :: Provider: CalDAV, defect)

Lightning 5.3
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mkmelin, Assigned: Fallen)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

I have two google caldav calendars set up, and now it's asking for OAuth2 authentication every time I start up Thunderbird. For the one calendar. The other one seems to work fine.

This is with nightly. I tried an old build, and at least 2016-08-10 is working fine.

I see this in the console, which might be of interest.

JavaScript error: resource://gre/components/nsLoginManager.js, line 445: NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService2.newURI]
Narrowed it down to 2016-08-10 -> 2016-08-15 (no linux nightly builds available in between)
What Thunderbird and Lightning version did you use for testing? Is the regression range for Thunderbird or for Lightning?
This is for 5.3.

There have been no related changes for calendar in that time. That was the period, when Aceman fixed all the compiler warnings.

Probably more related: There are currently also test failures related to the password manager (see bug 1301109), so it seems there have been some related changes in MC. Maybe this is a fallout. (I haven't checked the push log of MC for that time range).
Version: unspecified → Lightning 5.3
Looks like a regression from bug 1058438.
Magnus, can you test whether this patch resolves the issue for you?
Attachment #8795769 - Flags: feedback?(mkmelin+mozilla)
Tried the patch. It still asks for the password.
Error log before entering the password:

Wed Sep 28 2016 20:30:58
Warning: ReferenceError: assignment to undeclared variable gaia
Source file: https://accounts.google.com/ServiceLogin?passive=1209600&continue=https://accounts.google.com/o/oauth2/auth?login_hint%3Drichard.marti...
Line: 1491
 ----------
Wed Sep 28 2016 20:30:58
Warning: ReferenceError: assignment to undeclared variable hashParams
Source file: https://accounts.google.com/ServiceLogin?passive=1209600&continue=https://accounts.google.com/o/oauth2/auth?login_hint%3Drichard.marti...
Line: 1517
 ----------
Wed Sep 28 2016 20:30:58
Warning: ReferenceError: assignment to undeclared variable gaia_onLoginSubmit
Source file: https://accounts.google.com/ServiceLogin?passive=1209600&continue=https://accounts.google.com/o/oauth2/auth?login_hint%3Drichard.marti...
Line: 1560
 ----------
Wed Sep 28 2016 20:30:58
Warning: ReferenceError: deprecated caller usage
Source file: https://accounts.google.com/ServiceLogin?passive=1209600&continue=https://accounts.google.com/o/oauth2/auth?login_hint%3Drichard.marti...
Line: 1

After log in I get this error counting up permanently:

Wed Sep 28 2016 20:35:38
Warning: ReferenceError: deprecated caller usage
Source file: https://accounts.google.com/AccountLoginInfo line 1492 > eval
Line: 1

and other syntax errors coming and going (can't catch them because the buffer is filled up by the previous error).
Thanks. These error messages all come from the google code - can you disable javascript.options.showInConsole to exclude all of the reference errors being logged? It's also only intersting what you get after entering the password - maybe you can clear the log before doing so.
There are only errors from the Google code, no local.
(In reply to Stefan Sitter from comment #2)
> What Thunderbird and Lightning version did you use for testing? Is the
> regression range for Thunderbird or for Lightning?

Lightning trunk + thunderbird trunk (going back and forwards in the regression range).
Comment on attachment 8795769 [details] [diff] [review]
FixRepeatedPasswordRequestForOAuth-V1.diff

Review of attachment 8795769 [details] [diff] [review]:
-----------------------------------------------------------------

Dunno if this is a correct change or not, but it doesn't help with the problem in this bug. Still get the auth prompt on startup.

Question: when building with --enable-calendar, is the built version of lightning installed already when running, or do I need to install it from a file in dist/?
Anyway, tried both ways and both versions prompted.
Attachment #8795769 - Flags: feedback?(mkmelin+mozilla)
(In reply to Magnus Melin from comment #14)
> Question: when building with --enable-calendar, is the built version of
> lightning installed already when running, or do I need to install it from a
> file in dist/?

For non-beta/release builds, the extension ends up in the extensions/ subfolder (not distribution), so as long as you don't have a lightning installed in your dev profile it will use the one built with. You will get the tab that asks you to enable Lightning the first time. When you make changes to Lightning I'd suggest make -C objdir-tb/calendar/lightning tools (or libs tools if you make jar changes) to make sure repackaging happens correctly, especaially on mac.
But when I already have lightning installed in the profile, I need to install from file?
In that case, just remove the profile installed Lightning and the globally installed one will surface. You may need to enable the global installed one afterwards.
Any idea when this is going to make it into a downloadable build somewhere?
Nathan, as long as there is no fix for the problem available this is unknown. The only workaround I can think of right now is downgrading from Earlybird 51.0a2 to Thunderbird 50 Beta that is not affected by this bug.
We should try to drill down and fix the issue here before 5.3 moves to beta.

Magnus, as requested in comment 17, did you made sure the patched version of Lightning wasn't overlayed by an over-installed unpatched version of Lightning existing in your profile when testing this?
Blocks: ltn54
Flags: needinfo?(mkmelin+mozilla)
Comment on attachment 8795769 [details] [diff] [review]
FixRepeatedPasswordRequestForOAuth-V1.diff

This throws an NS_ERROR_MALFORMED_URI error with a Google calendar, using OAuth the token is stored using the calendar id as the host name. I was able to get it working by removing the getLoginSavingEnabled check altogether, do we really need that here?
Flags: needinfo?(makemyday)
Thanks for taking a look. It's not quite suprising that removing that line of code as that is calling the code that was changed by the referenced bug. I'm not an expert for that part of the code and maybe Philipp nows better, but it looks like this check is ment to avoid to throw an exception if the user hasn't ticked the checkbox to keep the password.

But if it works without for the use cases with and without stored password and using/not using the master password this should be fine.
Flags: needinfo?(makemyday)
I tried the patch from Matthew, and I still get the prompt. That part of the code doesn't appear to be called. I'm sure I had the patch applied as I repeated the test and put some junk in the file (which was noticed).
Flags: needinfo?(mkmelin+mozilla)
Summary: Google caldav calendar doesn't remember authentication anymore → Google calendar authentication not remembered anymore / prompts for access on each Thunderbird startup
Attached patch Fix - v1 (obsolete) β€” β€” Splinter Review
That function requires an origin, this patch creates one. I'm not sure about Magnus' error case though, if this code is not called...
Assignee: nobody → philipp
Attachment #8795769 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8813916 - Flags: review?(makemyday)
Comment on attachment 8813916 [details] [diff] [review]
Fix - v1

Magnus, can you test this patch? If it does not work, maybe you can attach the debugger and check the call stack for that error message you were getting.
Attachment #8813916 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 8813916 [details] [diff] [review]
Fix - v1

Review of attachment 8813916 [details] [diff] [review]:
-----------------------------------------------------------------

It's clear that we must pass in an origin,

::: calendar/base/modules/calAuthUtils.jsm
@@ +116,5 @@
>          try {
> +            let origin = aHostName;
> +            if (!origin.startsWith("http")) {
> +                origin = "https://" + aHostName;
> +            }

While it's obvious we need to pass an origin here, I'm not sure this is appropriate to prepare it.
 
Based on you only append the prefeix conditionally, you expect it to be part of aHostname in some cases already. If you create a calendar using the wizard, it will not allow you to enter an URI without a protocoll prefeix iirc. That said, if you would have entered e.g. webcal://domain.tld/something, wouldn't you end up in passing https://webcal://domain.tld/something here?

Apart from that, aren't the passwords stored separately for http and https? If so, I would expect you need to just prepend http instead of https here for backwards compatibility reasons. Or would backwards compatibility be broken anyway by the upstream change of the storage location?
Comment on attachment 8813916 [details] [diff] [review]
Fix - v1

Review of attachment 8813916 [details] [diff] [review]:
-----------------------------------------------------------------

Yep this fixes it for me. FTR, aHostName is apparently a uuid here.
Attachment #8813916 - Flags: feedback?(mkmelin+mozilla) → feedback+
I had this issue and found this report. I have done the fix from 8813916 and this works for me with no other changes done.
Does the fix appear in 51.0b1 build4?
(In reply to Arthur K. from comment #31)
> Does the fix appear in 51.0b1 build4?

No, this hasn't landed yet.
More importantly: The patch hasn't been reviewed yet. After review, it gets committed to the code base (we call this "to land"). Then the bug is marked "resolved" and then, maybe, the patch can also be included into current alpha/Aurora or beta versions.
Attached patch Fix - v2 (obsolete) β€” β€” Splinter Review
The patch has not been reviewed and the bug has not been marked fixed, and the patches have not been uplifted, therefore it is not in the beta.

Here is an updated patch that solves it slightly better. I've left in compatibility for passing a hostname/token instead of an origin, so that the upgrade is seamless for the Provider for Google Calendar. We can easily remove this later on.

There is no common sensible origin for our cases, so I've went with using a mock protocol oauth:<sessionId> for the caldav/gdata cases, falling back to https://<hostname> for the general case. This will cause older gdata providers on a newer Lightning to use e.g. https://cad7d7de-a2e5-4f9d-ac7c-7ade2db246d7 but given that does not have a tld I think it would be safe.

With the extra complexity it might be worth just dropping the _ensureOrigin code in calAuthUtils though.

As we are changing the origin here to be an uri instead of a hostname there will most likely be some migration issues causing passwords to have to be reentered. I personally despise migration code and would prefer users to live with it by entering their password once. The only alternatives I see are to:

1) Only use an origin for the function that actually enforces one, possibly breaking things in the future
2) Write migration code that finds logins without an origin and converts them

What do you think?
Attachment #8813916 - Attachment is obsolete: true
Attachment #8813916 - Flags: review?(makemyday)
Attachment #8818289 - Flags: review?(makemyday)
Comment on attachment 8818289 [details] [diff] [review]
Fix - v2

Review of attachment 8818289 [details] [diff] [review]:
-----------------------------------------------------------------

Apart from the comments below, this looks good. r+ with that cross checked.

::: calendar/providers/caldav/calDavCalendar.js
@@ +1630,5 @@
>                          return this.mRefreshToken;
>                      },
>                      set: function(val) {
>                          try {
> +                            let origin = "data:" + sessionId;

Why are you using "data:" instead of "oauth:" here like everywhere else?

::: calendar/providers/gdata/modules/gdataSession.jsm
@@ +164,5 @@
>                  return this.mRefreshToken;
>              },
>              set: function setRefreshToken(val) {
>                  try {
> +                    let origin = "data:" + sessionId;

same here
Attachment #8818289 - Flags: review?(makemyday) → review+
Attached patch Fix - v3 β€” β€” Splinter Review
I missed committing those parts before I exported the patch! Good catch.
Attachment #8818289 - Attachment is obsolete: true
Attachment #8818442 - Flags: review+
Attachment #8818442 - Flags: approval-calendar-beta+
Attachment #8818442 - Flags: approval-calendar-aurora+
https://hg.mozilla.org/comm-central/rev/2d60cf460eeb20c2bb80c26884cf128c60de9f53
Uplift coming.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 5.5
Thanks for doing the aurora checkin. This still needs uplift to beta when convenient.
Keywords: checkin-needed
I know. It's on the radar. But since beta is busted, there is no rush. BTW, checkin-needed is for C-C only.
Keywords: checkin-needed
As calendar has no tracking flags (and no intention to use them), we do use checkin-needed for aurora/beta. As long as it is on your radar I'm fine with skipping it this time. Thanks for taking care!
I have four queries: Aurora approval needed, Aurora check-in needed, and the same for beta. I adapted these queries to also pick up Calendar changes, I even found bug 1310440 where uplift was granted but never executed. Trust me, I'm pedantic ;-) My query for "checkin-needed" wouldn't have found the bug since it looks at open bugs only. 

This bug is currently the only bug waiting for beta uplift. I guess when bug 1176399 is ready, they will both go.
Verified working on trunk. Did have to enter the password again the first time (only), but as I understand it that was expected.
Status: RESOLVED → VERIFIED
Beta (TB 51/Calendar 5.3):
https://hg.mozilla.org/releases/comm-beta/rev/ecce0f2803565cc523c8971ca5e95522f9ec4753
Merry Christmas!
Target Milestone: 5.4 → 5.3
Sorry for a question that I feel I should already know, but how does one get this update? I am using the current beta from the beta channel, 51b1 (https://launchpad.net/~mozillateam/+archive/ubuntu/thunderbird-next). I checked the Aurors channel (https://launchpad.net/~ubuntu-mozilla-daily/+archive/ubuntu/thunderbird-aurora), but there has not been an update there in along while. So, I assume that either I am using the wrong places to get updates or I am missing something more.
Thanks
(In reply to Jorg K (GMT+1) from comment #44)
> Beta (TB 51/Calendar 5.3):
> https://hg.mozilla.org/releases/comm-beta/rev/
> ecce0f2803565cc523c8971ca5e95522f9ec4753
> Merry Christmas!

Any chance we can get something to test with soon?
(In reply to Arthur K. from comment #46)
> Any chance we can get something to test with soon?
Sadly our beta builds are currently broken, check bug 1317863. But you can use a build from the Earlybird channel: http://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-aurora/
(In reply to Jorg K (GMT+1) from comment #47)
> (In reply to Arthur K. from comment #46)
> > Any chance we can get something to test with soon?
> Sadly our beta builds are currently broken, check bug 1317863. But you can
> use a build from the Earlybird channel:
> http://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-aurora/

jorgk, if you uplift this to beta *RSN* we can take this in beta 2.
If it's not you please redirect
Flags: needinfo?(jorgk)
This was already in the current b1. Uplift is in comment 44.
Flags: needinfo?(jorgk)
Already uplifted to beta, see comment #44. Should already be included in TB 51 beta.

What do mean by RSN (real soon now)? I have other uplifts. OK, I'll do them in the next hour.
Thanks. I misunderstood the comments. It came to my attention because some bloke posted today in m.s.Thunderbird "TB 51.0b1, don't update unless you want to break your calendar ... FYI: https://bugzilla.mozilla.org/show_bug.cgi?id=1301422 is not yet fixed in this build. Tried it myself. Horkage abounds. Reverted back to 50.0b3."
Fix confirmed in Tb 51.0b2-build1 + Lightning 5.3b2-build1 :)
(In reply to Ronan Jouchet from comment #52)
> Fix confirmed in Tb 51.0b2-build1 + Lightning 5.3b2-build1 :)

So, you want the bad news or you want the bad news? Tested this build and it's still broken. =(
You want me to post error console info?
(In reply to Ronan Jouchet from comment #52)
> Fix confirmed in Tb 51.0b2-build1 + Lightning 5.3b2-build1 :)

Hang on, I erred. While on first run it *did* prompt for credentials, after re-entering username+PW for Lightning clicking "Allow" in the Google prompt window made the setting stick. I tested between OAuth2 and Normal Password and it seems to be working as it should. Looks fixed to me. Sorry for the alarm. WFM.
In my case the issue is not fixed and the prompt apeears every time I start Thunderbird.
I use Thunderbird 52.4.0,Lightning 5.4.4 and Google provider 3.3
I am using Ubuntu 17.10
I enabled cookies in Thunderbird settings.
I have also tried with a completely fresh Thunderbird profile.

Could it be related to the fact that I use KeeFox add-on ?
Please test with any addon but Lightning and the Google provider disabled. If this is still an issue then, please file a new bug.
This bug may come back to bite users of TB 60 as I have seen this with TB 60b4 using Provider 4.1b4 and Lightning 6.2b5. I just updated to TB 60b5 and it began prompting me repeatedly for allowing TB to manage my gmail calendars.
Testing with TB60.0b8, it seems it has the final Lighting 6.2 bundled. The only version of Provider for Google Calendar which doesn't have this repeated prompting issue is 4.1b3. Any way to use mozgression to figure out what change causes the problem?
I have created bug 1470424 as a clone of this bug with Provider for Google Calendar being the component with bustage.

I'm experiencing this problem with Thunderbird 68.6.0.

Could it be a regression?

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

Attachment

General

Creator:
Created:
Updated:
Size: