Closed Bug 1844810 Opened 2 years ago Closed 2 years ago

OAuth2 for comcast.net

Categories

(Thunderbird :: Account Manager, enhancement)

Product:
Thunderbird
Component:
Account Manager
Version:
Thunderbird 121
Type:
enhancement

Tracking

(thunderbird_esr115 fixed)

Status:
RESOLVED FIXED
Milestone:
122 Branch
Tracking Flags:
Tracking Status
thunderbird_esr115 --- fixed

People

(Reporter: atbrotman, Assigned: cketti)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

OAuth2Providers.jsm
9.51 KB, application/javascript
Details
Bug 1844810 - Add OAuth credentials for Comcast. r=mkmelin
48 bytes, text/x-phabricator-request
wsmwk
: approval-comm-esr115+
Details | Review
Attached file OAuth2Providers.jsm —

Steps to reproduce:

Attempting to add OAuth2 as a login method for comcast.net users

Actual results:

Works when tested in a local build

Expected results:

Users should be able to login via OAuth2

Assignee: nobody → atbrotman
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Thunderbird 116

Will this be included as part of 116 GA, or will it be a later version?

Alex: Does your system require Thunderbird to request the "openid" and "profile" scopes, or would the email scope itself ("https://email.comcast.net/") be sufficient? Could you test a local build without those scopes?

Flags: needinfo?(atbrotman)

cketti: I've been told it's as we specified. Does that cause a problem?

Flags: needinfo?(atbrotman) → needinfo?(cketti)

It could be a user experience problem. Depending on what Comcast's prompt to the user looks like, they could be asked to grant access to data that an email app probably shouldn't have access to. This could lead to unnecessary support requests.

The "openid" and "profile" scopes are part of OpenID Connect (and shouldn't be necessary). The "profile" scope is specified to give access to a bunch of personal data:
name, family_name, given_name, middle_name, nickname, preferred_username, profile, picture, website, gender, birthdate, zoneinfo, locale, and updated_at (see https://openid.net/specs/openid-connect-basic-1_0.html#Scopes)

If not absolutely necessary on your end, I'd like to avoid requesting those scopes in Thunderbird.

If this goes into the daily build for Tbird, I can give it a try with just the first scope and ensure it works properly. Sound agreeable?

Flags: needinfo?(cketti)
Status: NEW → ASSIGNED
Keywords: checkin-needed-tb
Version: Thunderbird 116 → Thunderbird 121

(In reply to Alex Brotman from comment #5)

If this goes into the daily build for Tbird, I can give it a try with just the first scope and ensure it works properly. Sound agreeable?

Hey Alex, yes, that would be nice, if you could test daily once it lands.

We cannot really answer which scopes are needed, because that's defined entirely by the OAuth2 server and the services using the token. The OAuth2 spec does not define it. We need only the scopes necessary to login and use IMAP, POP3, SMTP. If you have calendar, contacts and WebDAV servers for your customers as well, we should include those scopes as well. As cketti said, and I agree, we should not request scopes (permissions) that we do not need.

As a side note, I do like the client ID that you used very much! :-)

FWIW, I just reviewed it as-is, so that this can land and you can test it. Note that this is an unusual approach and we normally expect the patch contributor to have tested it first, but I understand that this would be extraordinarily difficult for you in this case, so let's do it the other way around this time, exceptionally.

It's easy enough to create a try build that can be tested. I've set one off now - https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=ea5ca22ce4581e5cc14d76bb065e070d04b08e31 (once finished, clicking on B and then artifacts will give you builds that can be downloaded for testing - I'll come back later and post direct links)

Also, would it be possible to provide a test account for the Thunderbird team? Otherwise if/when something goes wrong in the future we can't debug.

Keywords: checkin-needed-tb

Please try a build from comment 9 and tell us if it works

Assignee: atbrotman → cketti
Flags: needinfo?(atbrotman)

It looks like retrieving works, but sending does not. When discussing with that Identity team, they noted that one of the pieces of information from openid/profile is validated upon login attempt and therefore is necessary for the SMTP transaction to be successful.

Seems like all three are necessary.

Happy to test another daily build with all three

Flags: needinfo?(atbrotman) → needinfo?(mkmelin+mozilla)

I've updated the patch on Phabricator. I don't think I have permissions to create a try build, though.

I can set one off once the current windows bustage clears up.

Flags: needinfo?(mkmelin+mozilla)

Seems good for me. Going to have a few others test these builds.

Flags: needinfo?(atbrotman)

Thanks, Alex.

Would it be in your power and possibility to create a test account for us? Magnus has a point that it would help to test the Comcast config, to check whether the config still works in the future, or to test changes in Thunderbird.

Flags: needinfo?(atbrotman)

Let me ask again (I asked right before the US holiday). It might be something that you have to use against a pre-prod endpoint, but seems like this should be doable.

Flags: needinfo?(atbrotman)

Remember that we have server names (imap.comcast.net, oauth.xfinity.com etc.) hardcoded.

As far as the application is concerned, the patch works properly on Windows and Mac.

I've inquired and created a ticket for test credentials.

I believe those hostnames match what we added to AutoConfig. Is there a problem with that?

As far as the application is concerned, the patch works properly on Windows and Mac.

Thanks for checking 👍

I believe those hostnames match what we added to AutoConfig. Is there a problem with that?

I believe Ben's comment was in regards to a test account possibly being for a "pre-prod endpoint". If a test account requires an OAuth configuration different from what you attached to this issue, we won't be able to use them without changing Thunderbird. The host names are part of the OAuth config. So if the pre-prod environment requires using e.g. imap.preprod.comcast.net, we won't be able to use OAuth (out of the box).

The pre-prod does indeed use a different end point. I've asked for a production UID

Thank you!

Keywords: checkin-needed-tb

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/cb3cfaff121e
Add OAuth credentials for Comcast. r=mkmelin,BenB

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED

How can I securely provide the credentials?

Alex: If you send me an email, I'll add the credentials to our team's password manager.

My OpenPGP key can be found on keys.openpgp.org: https://keys.openpgp.org/search?q=cketti@thunderbird.net

cketti: Sent you an "encrypted" mail (it's the way corporate allows) to you. Let me know if you didn't get that.

Uplift to 115?

status-thunderbird_esr115: --- → affected
Target Milestone: --- → 122 Branch

That was the version I originally tested with. So that's fine with me (if my opinion matters)

This should be fine to uplift. For now it will only affect users that manually configure their Comcast account in Thunderbird to use OAuth.

We'll update ISPDB (see https://github.com/thunderbird/autoconfig/pull/73) once most users use a version that either supports OAuth with Comcast or ignores unsupported OAuth settings in the autoconfig file (see bug 1869122).

Attachment #9364844 - Flags: approval-comm-esr115?

Comment on attachment 9364844 [details]
Bug 1844810 - Add OAuth credentials for Comcast. r=mkmelin

[Triage Comment]
Approved for esr115

Attachment #9364844 - Flags: approval-comm-esr115? → approval-comm-esr115+
See Also: → 1869122
Blocks: tb-enterprise, 1310389
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: