1909789 - Account setup can't handle partially granted OAuth2 scopes (gmail-related)

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, enhancement)

Description

[Geoff Lankow (:darktrojan)]

Google is, so I'm told, going to enable OAuth2 granular permissions for Thunderbird. Our code can sort-of handle these, but we record the scopes we requested, not the ones we got. This shouldn't happen. If we save an access token and claim it has (e.g.) the CardDAV permission, but it doesn't, the CardDAV code gets confused and is unable to find any address books.

I've written a patch to fix this. To be thorough, here's what we need to test:

• When setting up a mail account, and
  • granting all permissions,
    • does the mail account work?
    • can address books and calendars be set up in the mail setup wizard?
    • can address books and calendars be set up separately after the mail setup wizard?
  • granting only the mail permission,
    • does the mail account work?
    • can address books and calendars be set up in the mail setup wizard? (no automatic attempt should be made)
    • can address books and calendars be set up separately after the mail setup wizard?
  • not granting the mail permission,
    • do we handle this like a cancellation of the prompt?
  • not granting any permissions but accepting the prompt,
    • do we handle this like a cancellation of the prompt?
• If we set up an address book or calendar first, can we still set up mail?

In all cases, these things should work after a restart, with no additional prompts.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1909789 - Adjust account setup to handle partial granting of OAuth2 permission. r=tobyp

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1909789 - Test that scopes modified during OAuth2 authentication are saved correctly. r=aleca,tobyp

Comment 3

[Alessandro Castellani [:aleca]]

We will want to uplift this for 115 as well otherwise we might break profiles before they can upgrade to 128

Comment 4

[Alessandro Castellani [:aleca]]

I tested the WIP patch pretty thoroughly and these are my findings.

(In reply to Geoff Lankow (:darktrojan) [away until August 27] from comment #0)

• When setting up a mail account, and
  • granting all permissions,
    • does the mail account work?

YES

can address books and calendars be set up in the mail setup wizard?

YES, no further permission request is triggered

can address books and calendars be set up separately *after* the mail setup wizard?

YES, no further permission request is triggered

• granting only the mail permission,
  • does the mail account work?

YES

can address books and calendars be set up in the mail setup wizard? (no automatic attempt should be made)

YES. No automatic attempt is made but if manually triggered the Google permission dialog appears only for the specific section (calendar or address book)

- can address books and calendars be set up separately *after* the mail setup wizard?

YES

• not granting the mail permission,
  • do we handle this like a cancellation of the prompt?

YES, we show a warning with authentication/permission error message.

• not granting any permissions but accepting the prompt,
  • do we handle this like a cancellation of the prompt?

YES, we show a warning with authentication/permission error message.

• If we set up an address book or calendar first, can we still set up mail?

YES.
The only strange thing with this last step is that when going through the email setup, Google doesn't seem to remember that I already granted permission to Contacts and Calendars, but even if I confirm the dialog without those 2 permissions unchecked, it doesn't seem to revoke them and everything still works.

Comment 5

[Alessandro Castellani [:aleca]]

I'll steal these patches since Geoff is on PTO and bring them to completion.
He can yell at me at MozWeek if I do something wrong.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8c451e882d81
Adjust account setup to handle partial granting of OAuth2 permission. r=tobyp

Comment 7

[Corey Bryant]

Comment on attachment 9414748 [details]
Bug 1909789 - Adjust account setup to handle partial granting of OAuth2 permission. r=tobyp

[Triage Comment]
Approved for beta
(2 patches)
This depends on fixes in bug 1912556 and bug 1912738.

Comment 8

[Francesco]

(In reply to Corey Bryant from comment #7)

Comment on attachment 9414748 [details]
Bug 1909789 - Adjust account setup to handle partial granting of OAuth2 permission. r=tobyp

[Triage Comment]
Approved for beta
(2 patches)

One patch hasn't been reviewed/landed and there are two regressions, bug 1912556 and bug 1912738. Shouldn't all this be backported together?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/af31a5278587
Test that scopes modified during OAuth2 authentication are saved correctly. r=aleca

Comment 10

[Rob Lemley [:rjl]]

Thunderbird 130.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/70f792b3b7d0
https://hg.mozilla.org/releases/comm-beta/rev/04273ac6d74b

Comment 11

[Corey Bryant]

One patch hasn't been reviewed/landed and there are two regressions, bug 1912556 and bug 1912738. Shouldn't all this be backported together?

Yes, that discussion was handled in matrix and not in the bug but I'll update the bugs to be clear. Thanks!

Comment 12

[Corey Bryant]

Comment on attachment 9414748 [details]
Bug 1909789 - Adjust account setup to handle partial granting of OAuth2 permission. r=tobyp

[Triage Comment]
Approved for esr128
Approved for esr115

Comment 13

[Rob Lemley [:rjl]]

Needs D210712's changes to OAuth2Module.sys.mjs to avoid merge conflicts. Possibly other things from bug 1896171 as well?

Comment 14

[Yury]

Attachment: 1909789-part1.patch (rebased to 128)

First part rebased to 128 codebase, two hunks needed a small and safe tweak. Part 2 as well as all the patches in bug 1912738 and bug 1912556 apply without issue.

Comment 15

[Brendan Abolivier [:babolivier]]

Thanks a lot for the patch, Yury! Could you submit it via Phabricator please? We don't handle Bugzilla patches anymore.

Comment 16

[Geoff Lankow (:darktrojan)]

Attachment: WIP: Bug 1909789 - Adjust account setup to handle partial granting of OAuth2 permission. (ESR128 version)

Comment 17

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Confirming this issue as verified fixed on 130.0b2(20240816160526) and 130.0b3(20240826151843) using macOS 14 , Windows 11 and Ubuntu 22 using the STR from comment 29 from bug 1912556.

Comment 18

[Corey Bryant]

Thanks Geoff and Yury. Once this is ready for uplift, can you submit the uplift request?

I'm going to remove the approval-comm-esr128+/approval-comm-esr115+ flags for now to avoid confusion.

Comment 19

[Corey Bryant]

Comment on attachment 9414748 [details]
Bug 1909789 - Adjust account setup to handle partial granting of OAuth2 permission. r=tobyp

[Triage Comment]
Awaiting new patches for 128esr and 115esr.

Comment 20

[Geoff Lankow (:darktrojan)]

Attachment: WIP: Bug 1909789 - Test that scopes modified during OAuth2 authentication are saved correctly. (ESR128 version)

Comment 21

[Geoff Lankow (:darktrojan)]

Comment on attachment 9420926 [details]
WIP: Bug 1909789 - Adjust account setup to handle partial granting of OAuth2 permission. (ESR128 version)

[Approval Request Comment]
Regression caused by (bug #): Google enabling granular consent screens.

User impact if declined: OAuth credentials are recorded badly, user will be unable to add address books/calendars after the original account setup

Testing completed (on c-c, etc.): In 130.0b2 and tested there by QA. But Google are not showing the granular consent screen to beta users. Gmail and Outlook checked by me with all five patches (this bug, bug 1912738, two from bug 1912556) applied and the user agent changed so the screens show.

Risk to taking this patch (and alternatives if risky): This makes me nervous but the lack of complaints from beta users is a good sign. Alternatively we could just not patch ESR and let broken profiles be a support issue. Perhaps Google will continue to not show the granular consent screen to ESR users?

Comment 22

[Geoff Lankow (:darktrojan)]

Comment on attachment 9421161 [details]
WIP: Bug 1909789 - Test that scopes modified during OAuth2 authentication are saved correctly. (ESR128 version)

[Approval Request Comment]
See comment 21.

Comment 23

[Corey Bryant]

Comment on attachment 9420926 [details]
WIP: Bug 1909789 - Adjust account setup to handle partial granting of OAuth2 permission. (ESR128 version)

[Triage Comment]
Approved for esr128.

Chatted with Geoff and while there is still risk, we'd like to get this in while the 128 user base is still somewhat low.

Comment 24

[Corey Bryant]

Comment on attachment 9421161 [details]
WIP: Bug 1909789 - Test that scopes modified during OAuth2 authentication are saved correctly. (ESR128 version)

[Triage Comment]
Approved for esr128

Comment 25

[Daniel Darnell [:dandarnell]]

Thunderbird 128.2.2esr:
https://hg.mozilla.org/releases/comm-esr128/rev/cb3d6ad20ca2
https://hg.mozilla.org/releases/comm-esr128/rev/ddc7f61e9109

Comment 26

[Daniel Darnell [:dandarnell]]

Backed out in Thunderbird 128.2.3esr:
https://hg.mozilla.org/releases/comm-esr128/rev/33d67ea25ab5
https://hg.mozilla.org/releases/comm-esr128/rev/830193ea5dee

Comment 27

[Geoff Lankow (:darktrojan)]

I'm setting this (and the two other bugs) back to fixed, as that's the normal state for bugs that have landed on central. When we manage to find and resolve the problems this caused on 128, I'll make one super-patch of all the changes as I'm tired of dragging 5 patches around.