Closed Bug 1079783 Opened 5 years ago Closed 5 years ago

OAuth2 forgets token when offline and a few other minor OAuth2.jsm fixes

Categories

(MailNews Core :: Backend, defect)

defect
Not set

Tracking

(thunderbird34 fixed, thunderbird35 fixed, thunderbird36 fixed, thunderbird_esr3134+ fixed)

RESOLVED FIXED
Thunderbird 36.0
Tracking Status
thunderbird34 --- fixed
thunderbird35 --- fixed
thunderbird36 --- fixed
thunderbird_esr31 34+ fixed

People

(Reporter: Fallen, Assigned: Fallen)

References

Details

Attachments

(1 file)

I have a patch to fix the following minor issues:

* Support "token" responseType
* Allow setting a dialog title for the OAuth2 dialog
* Allow changing the dialog window features
* Move auth uri setter outside of constructor
* Allow passing extra auth parameters, like Google's login_hint
* Fix indentation on two functions
* Handle servers that don't send an expires time

But the most important fix is about the offline mode. There are situations where the refresh token is nulled out because the request is being made in limbo between Mozilla detecting that it has left offline mode and the network being available. If the refreshToken setter is hooked up to the password manager this means the login dialog will reappear once in a while.

I'd like to backport this to Thunderbird 31 so that Google CalDAV v2 users have less pain. If the other changes are too massive, I would at least like to see the window title and offline chunks backported.
Attached patch Fix - v1 β€” β€” Splinter Review
Here is the patch. I've added Patrick as the main reviewer and Mark since he is a mailnews reviewer for a rs and the approvals.


[Approval Request Comment]
Regression caused by (bug #): --
User impact if declined: Constant relogins after resuming from suspend
Testing completed (on c-c, etc.): Local testing with box.com and Google CalDAV
Risk to taking this patch (and alternatives if risky):
  - Possible regressions with some server types
  - Alternative is to take only two chunks as mentioned.
Attachment #8501655 - Flags: review?(standard8)
Attachment #8501655 - Flags: review?(clokep)
Attachment #8501655 - Flags: approval-comm-esr31?
Attachment #8501655 - Flags: approval-comm-beta?
Attachment #8501655 - Flags: approval-comm-aurora?
Duplicate of this bug: 901331
The calendar bits bascially have r+ from bug 901331.
Comment on attachment 8501655 [details] [diff] [review]
Fix - v1

I don't know this code at all. I think Mike does.
Attachment #8501655 - Flags: review?(standard8) → review?(mconley)
(In reply to Philipp Kewisch [:Fallen] from comment #0)
> * Allow setting a dialog title for the OAuth2 dialog
> * Allow changing the dialog window features
These are nice changes, thanks!

> * Allow passing extra auth parameters, like Google's login_hint
I see you added this feature, but you don't use it. Did you mean to?

Overall these changes look good, but I'd like an answer to the above!
(In reply to Patrick Cloke [:clokep] from comment #5)
> (In reply to Philipp Kewisch [:Fallen] from comment #0)
> > * Allow passing extra auth parameters, like Google's login_hint
> I see you added this feature, but you don't use it. Did you mean to?
I am using this in the Provider for Google Calendar, which for now is packaging its own version but I'd like to drop support for TB24 soon and with that use the builtin OAuth2.jsm. For CalDAV I could assume the organizerId, but for the first login it won't be known and for secondary calendars it will be incorrect.
Comment on attachment 8501655 [details] [diff] [review]
Fix - v1

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

This looks good to me! Unfortunately my build is busted right now so I can't test this, but the changes are fairly trivial. I can't officially review mailnews code, but I'll leave it to Mike decide if he wants to do a full review or rubber-stamp it.
Attachment #8501655 - Flags: review?(clokep) → review+
Comment on attachment 8501655 [details] [diff] [review]
Fix - v1

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

The changes to nsBox.js look good to me. Thanks!
Attachment #8501655 - Flags: review?(mconley) → review+
Duplicate of this bug: 1088606
Magnus, just wondering if you will be doing these approvals in the future?
Flags: needinfo?(mkmelin+mozilla)
Things should at least land on trunk before we grant approval - there might be problems that landing on trunk show up, so we'd have to go around the approval loop again.
Landed on trunk: https://hg.mozilla.org/comm-central/rev/5df70cd99cf7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0
Flags: needinfo?(mkmelin+mozilla)
Target Milestone: Thunderbird 38.0 → Thunderbird 36.0
(In reply to Philipp Kewisch [:Fallen] from comment #10)
> Magnus, just wondering if you will be doing these approvals in the future?

At some point, probably. We have no firm handover decisions yet.
Attachment #8501655 - Flags: approval-comm-beta?
Attachment #8501655 - Flags: approval-comm-beta+
Attachment #8501655 - Flags: approval-comm-aurora?
Attachment #8501655 - Flags: approval-comm-aurora+
approval ping? I'd like to see this in the next 31.x release.
Attachment #8501655 - Flags: approval-comm-esr31? → approval-comm-esr31+
You need to log in before you can comment on or make changes to this bug.