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

VERIFIED FIXED in 5.3

Status

Calendar
Provider: CalDAV
VERIFIED FIXED
a year ago
2 months ago

People

(Reporter: Magnus Melin, Assigned: Fallen)

Tracking

({regression})

Lightning 5.3
regression

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

a year ago
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]
(Reporter)

Comment 1

a year ago
Narrowed it down to 2016-08-10 -> 2016-08-15 (no linux nightly builds available in between)
(Reporter)

Updated

a year ago
Keywords: regressionwindow-wanted

Comment 2

a year ago
What Thunderbird and Lightning version did you use for testing? Is the regression range for Thunderbird or for Lightning?

Comment 3

a year ago
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

Comment 5

a year ago
Looks like a regression from bug 1058438.

Updated

a year ago
Duplicate of this bug: 1304840

Comment 7

a year ago
Created attachment 8795769 [details] [diff] [review]
FixRepeatedPasswordRequestForOAuth-V1.diff

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

Comment 10

a year ago
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.

Updated

a year ago
Duplicate of this bug: 1307409
(Reporter)

Comment 13

a year ago
(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).
(Reporter)

Comment 14

a year ago
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)
(Assignee)

Comment 15

a year ago
(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.
(Reporter)

Comment 16

a year ago
But when I already have lightning installed in the profile, I need to install from file?
(Assignee)

Comment 17

a year ago
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.

Comment 18

a year ago
Any idea when this is going to make it into a downloadable build somewhere?

Comment 19

a year ago
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.

Comment 20

a year ago
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: 1312113
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)

Comment 22

a year ago
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)
(Reporter)

Comment 23

a year ago
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)

Updated

a year ago
Duplicate of this bug: 1317975

Updated

a year ago
Summary: Google caldav calendar doesn't remember authentication anymore → Google calendar authentication not remembered anymore / prompts for access on each Thunderbird startup

Updated

a year ago
Duplicate of this bug: 1318205
(Assignee)

Comment 26

a year ago
Created attachment 8813916 [details] [diff] [review]
Fix - v1

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)
(Assignee)

Comment 27

a year ago
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 28

a year ago
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?
(Reporter)

Comment 29

a year ago
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+

Comment 30

a year ago
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.

Comment 31

a year ago
Does the fix appear in 51.0b1 build4?

Comment 32

a year ago
(In reply to Arthur K. from comment #31)
> Does the fix appear in 51.0b1 build4?

No, this hasn't landed yet.

Comment 33

a year ago
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.
(Assignee)

Comment 34

a year ago
Created attachment 8818289 [details] [diff] [review]
Fix - v2

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 35

a year ago
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+
(Assignee)

Comment 36

a year ago
Created attachment 8818442 [details] [diff] [review]
Fix - v3

I missed committing those parts before I exported the patch! Good catch.
Attachment #8818289 - Attachment is obsolete: true
Attachment #8818442 - Flags: review+
(Assignee)

Updated

a year ago
Attachment #8818442 - Flags: approval-calendar-beta+
Attachment #8818442 - Flags: approval-calendar-aurora+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 37

a year ago
https://hg.mozilla.org/comm-central/rev/2d60cf460eeb20c2bb80c26884cf128c60de9f53
Uplift coming.
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 5.5

Comment 38

a year ago
Aurora (Calendar 5.4/TB 52)
https://hg.mozilla.org/releases/comm-aurora/rev/b329c4342ae7d0df9d091c105d76b9b33eb0c63b
Target Milestone: 5.5 → 5.4
(Assignee)

Comment 39

a year ago
Thanks for doing the aurora checkin. This still needs uplift to beta when convenient.
Keywords: checkin-needed

Comment 40

a year ago
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
(Assignee)

Comment 41

a year ago
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!

Comment 42

a year ago
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.
(Reporter)

Comment 43

a year ago
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

Comment 44

a year ago
Beta (TB 51/Calendar 5.3):
https://hg.mozilla.org/releases/comm-beta/rev/ecce0f2803565cc523c8971ca5e95522f9ec4753
Merry Christmas!
Target Milestone: 5.4 → 5.3

Comment 45

a year ago
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

Comment 46

a year ago
(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?

Comment 47

a year ago
(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)

Comment 49

a year ago
This was already in the current b1. Uplift is in comment 44.
Flags: needinfo?(jorgk)

Comment 50

a year ago
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."

Comment 52

a year ago
Fix confirmed in Tb 51.0b2-build1 + Lightning 5.3b2-build1 :)

Comment 53

a year ago
(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?

Comment 54

a year ago
(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.

Comment 55

2 months ago
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 ?

Comment 56

2 months ago
Please test with any addon but Lightning and the Google provider disabled. If this is still an issue then, please file a new bug.
You need to log in before you can comment on or make changes to this bug.