1476330 - [Lightning, Provider: GData]: use stored oauth when the password manager is disabled

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[adrianf0]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180704194937

Steps to reproduce:

Setup:
 * Ubuntu 16.04 LTS
 * Thunderbird 52.9.1 

More details regarding the issue can be found under https://github.com/kee-org/keebird/issues/14


Actual results:


It seems, that in case the password manager is disabled (i.e. by the KeeBird plugin), Lightning/GData will always pop up Oauth2 login window instead of using the OAuth token stored during the previous session. 

The method with the stored OAuth2 and running KeeBird works properly with Google e-mail accounts served directly by Thunderbird. However, for Google calendars, Lightning/GData behaves differently than Thunderbird causing problems. 


Expected results:

Would it be possible that Lightning to use the built-in OAuth of Thunderbird?

Comment 2

[stefan.mueller.83]

Same problem here, see https://bugzilla.mozilla.org/show_bug.cgi?id=1154256

Comment 3

[[:MakeMyDay]]

Please don't file new reports for the same issue you already did (bug bug 1154256) but expand the existing one if you have new information. Filing a new bug helps no one and will not catalyse a fix. The same is true for me too comments.

Comment 4

[Stefan Sitter [:ssitter]]

Does the recently released Thunderbird 60 + Lightning 6.2 + GData Provider 4.4.1 show the same behavior?

Comment 5

[adrianf0]

I use the latest Thunderbord available through Mozilla PPA (http://ppa.launchpad.net/ubuntu-mozilla-security/ppa/ubuntu). Here Thunderbird is in version 52.9.1. When it comes to Lightning provided for Ubuntu 16.04 LTS, it's in version 5.4. However, also on the Thunderbird add-ons webpage (https://addons.thunderbird.net/en-GB/thunderbird/addon/lightning/?src=ss) the latest version is the one I use. Moreover, in comments, I noticed some incompatibility issues reported by users between Lightning and Thunderbird 60 mentioned by you.

To summarise: Thunderbird 52.9.1, Lightning 5.4, GData Provider 4.4.1 shows the same behaviour.

Comment 6

[stefan.mueller.83]

I removed all my calendars and email accounts and added them all again but it still shows the same behavior

60.0b10 (32-bit)
Lightning 6.2b6
Provider for Google Calendar 4.1.1

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v1

Untested, but this should do it. Adrian, is there a chance you could test this patch?

Comment 8

[adrianf0]

@Philip(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #7)
> Created attachment 9005797 [details] [diff] [review]
> Fix - v1
> 
> Untested, but this should do it. Adrian, is there a chance you could test
> this patch?

I have successfully applied the patch. It does not fix the issue. 
Tested with Tunderbird 52.9.1. I confirm the oAuth token for the google calendar was in the saved passwords.

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9005797 [details] [diff] [review]
Fix - v1

Ok, I think I misunderstood the report. I'll have to spend some more time looking into this.

Do I understand correctly that you have a saved token in the password manager, but if rememberSignons is then disabled it doesn't read from the password manager and hence pops up the password dialog every restart?

Comment 10

[adrianf0]

The idea is to store all credentials in one save place (database of KeePass). The oAuth2 tokens are not saved by KeeBird. It works perfectly with google e-mail accounts: the oAuth2 tokens are stored and fetched directly from the password manager. Here, I miss implementation details, as according to my knowledge the password manager supposed to be disabled when KeeBird is active. Maybe it's implemented such, that only oAuth2 tokens are served by a regular password manager (as I can find them only the tokens). I encourage you to check a KeeBird thread I linked to the report. 

The problem is that in this configuration, Lightning (GDataProvider) doesn't fetch a token from the password manager. Instead it asks for all credentials each time Thunderbird is launched, what is annoying.

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

The code around the area of the patch does actually retrieve the OAuth token from the password manager, though I am not sure why it asks every time. Maybe the author of KeeBird can weigh in here.

Comment 12

[David Lechner (:dlech)]

It sounds like you have it figured out. The KeeBird extension sets signon.rememberSignons to false so that no new passwords will be saved to the built-in password manager in Thunderbird. It appears that for mail accounts, when signon.rememberSignons is false, existing passwords (or in this case oauth tokens) can be retrieved from the built-in password manager. However, it appears that for calendar accounts, if signon.rememberSignons is set to false, then existing oauth tokens in the built-in password manager are ignored, which causes the prompt to re-enter credentials.

The patch looks like the right sort of thing to do, but I'm not seeing the related source code in comm-central. The only use of signon.rememberSignons I could find in calendar is this: https://dxr.mozilla.org/comm-central/source/calendar/base/modules/utils/calAuthUtils.jsm#193. Am I looking in the wrong place?

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

The relevant code is probably here, where the token is not retrieved if password saving is disabled for the origin:
https://dxr.mozilla.org/comm-central/source/calendar/base/modules/utils/calAuthUtils.jsm#287

We'll have to check if the login manager blocks on the pref, and then decide if we can skip the check here.

Comment 14

[David Lechner (:dlech)]

Thanks for the pointer. I'm guessing the check can be skipped because it isn't present, for example, in IM accounts.

Comment 15

[stefan.mueller.83]

Thunderbird 61 beta 11 still have the issue running on Windows 10 but TB's password manager is enabled by default settings

Comment 16

[thibault]

I did some testing on this matter. Changing the line 
            if (!Services.logins.getLoginSavingEnabled(origin)) {

to 
            if (!aOrigin.startsWith('oauth:') && !Services.logins.getLoginSavingEnabled(origin)) {

in modules/utils/calAuthUtils.jsm line 291 seems to do the trick, I do not have the Google Oauth popup on startup.

Since the code in gdataSession ignores the rememberSignons setting and always set the token in the password manager, it makes sense to ignore the setting when retrieving it, and we can easily detect the oauth case with the prefix in the origin. 

What do you think ?

Comment 17

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v2

Ok, understanding better now. You are right, it seems wrong to check for rememberSignons in the getter, as we are not changing anything there. Funny enough we are not checking in the save function.

I've changed the save function to throw, which might be troublesome for some add-ons that use the cal.auth.* functions. The internal callers are all modified correctly.

Comment 18

[[:MakeMyDay]]

Comment on attachment 9024241 [details] [diff] [review]
Fix - v2

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

I guees the Services.logins.getLoginSavingEnabled(origin) check was just intended to spare the subsequent search if saving isn't enabled, but removing it is fine.

Can you add also a small test set using an oauth: origin since we have a special case handling for it in _ensureOrigin? Apart from that just some nits, r+ with that fixed. A try push before landing would be nice.

::: calendar/providers/caldav/calDavCalendar.js
@@ +1645,5 @@
>                          } catch (e) {
> +                            // User might have cancelled the master password prompt, or password saving
> +                            // could be disabled. That is ok, throw for everything else
> +                            if (e.result != Components.results.NS_ERROR_ABORT &&
> +                                e.result != Components.results.NS_ERROR_NOT_AVAILABLE) {

Can you please change Components.results. to Cr. here (and at other places) when you're touching this?

::: calendar/test/unit/test_auth_utils.js
@@ +11,5 @@
> +    do_get_profile();
> +    run_next_test();
> +}
> +
> +function checkLoginCount(loginCount, delta=0) {

Can you change the second argument to expect an absolute value instead of a relative. That would make the test more readable I think.

@@ +21,5 @@
> +add_task(async function test_password_manager() {
> +    await Services.logins.initializationPromise;
> +
> +    // eslint-disable-next-line no-unused-vars
> +    let loginCount = Services.logins.findLogins({}, ORIGIN, null, REALM);

For type consistency, you should either pass in Services.logins.findLogins({}, ORIGIN, null, REALM).length here or simply pass 0 as first argument since you call Services.logins.findLogins in checkLoginCount anyway.

Comment 19

[[:MakeMyDay]]

One more thing. Can you please add a comment to test_password_manager() mentioning for which functions of cal.auth.* it claims to provide (complete) test coverage (or the opposite if you like, which still need some attention)?

Comment 20

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to [:MakeMyDay] from comment #18)
> Can you add also a small test set using an oauth: origin since we have a
> special case handling for it in _ensureOrigin? Apart from that just some
> nits, r+ with that fixed. A try push before landing would be nice.

Done, I think I covered all _ensureOrigins cases.


> Can you please change Components.results. to Cr. here (and at other places)
> when you're touching this?

I'd rather do this automated in whatever that bug was where the script runs instead of adding unrelated line changes to this patch. I suspect there are a few more in the vicinity.


> 
> @@ +21,5 @@
> > +add_task(async function test_password_manager() {
> > +    await Services.logins.initializationPromise;
> > +
> > +    // eslint-disable-next-line no-unused-vars
> > +    let loginCount = Services.logins.findLogins({}, ORIGIN, null, REALM);
> 
> For type consistency, you should either pass in
> Services.logins.findLogins({}, ORIGIN, null, REALM).length here or simply
> pass 0 as first argument since you call Services.logins.findLogins in
> checkLoginCount anyway.

Ah yes, that is a bug. I did want to use .length. Thanks for catching it! I've since revamped to use countLogins instead.

Comment 21

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v2

Looks like my hg access is disabled, I'll take care of a try run once it is back. If someone else wants to do this I'd appreciate!

Comment 22

[Philipp Kewisch [:Fallen][📱📆]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=159cab152bcb15a6de0f82e2d2064800f5290633

Comment 23

[Philipp Kewisch [:Fallen][📱📆]]

Try looks good, ready to go.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0eb273edd4ab
Get but don't save passwords if password saving is disabled. r=MakeMyDay

Comment 25

[adrianf0]

Hi,
I have noticed, that a fix has been pushed 9 months ago, however, despite regular updates, I am still affected by the issue.
My latest setup:
TB: 60.8.0 (64-bit)
KeeBird: 1.1.3
Google Provider: 4.4.2
Lightning: 6.2.8
OS: Uubntu 18.04 LTS
Does the fix requires any action on user's side?

Comment 26

[Jorg K (CEST = GMT+2)]

If you consult https://addons.thunderbird.net/en-GB/thunderbird/addon/provider-for-google-calendar/, version 4.4.2 was updated in Sept. 2018, so it can possibly not contain this fix. There's also a support e-mail at the add-ons website.

Comment 27

[adrianf0]

Thanks. I have just sent an e-mail with addon update request.

Comment 28

[Philipp Kewisch [:Fallen][📱📆]]

Latest version of the Provider is 68.0, this should be done

Comment 29

[adrianf0]

Unfortunately, I can't confirm on my side: Ubuntu 18.04 LTS (and PPA for Ubuntu Mozilla Security Team) provides only Thunderbird 60.08.0.

Comment 30

[adrianf0]

It is still a problem on Ubuntu 18.04 LTS:

• Thunderbird 68.2.1
• Lightning 68.2.1
• GData 68
• KeeBirid 2.0.0

Comment 31

[Jorg K (CEST = GMT+2)]

Please file an issue at https://github.com/kewisch/gdata-provider

Comment 32

[adrianf0]

Filled, thanks.