OAuth2.jsm should not send client_secret, if none is specified
Categories
(Thunderbird :: General, defect)
Tracking
(thunderbird_esr68 fixed)
Tracking | Status | |
---|---|---|
thunderbird_esr68 | --- | fixed |
People
(Reporter: TbSync, Assigned: TbSync)
Details
Attachments
(1 file, 4 obsolete files)
2.73 KB,
patch
|
mkmelin
:
review+
mkmelin
:
approval-comm-esr68+
|
Details | Diff | Splinter Review |
The current OAuth2.jsm is very much optimized for the Google v1 endpoints. Many things can be fixed for example for Office365 or the Google v2 endpoints by simply overiding internal values after an OAuth2 Object has been created (like authURI, tokenURI and completionURI).
One thing I cannot simply change is the inclusion of the client_secret, which is not optional with OAuth2.jsm. However Office365 does not require an client_secret in native apps and will actually not work if one is present.
TL;DR: You shouldn't use the application secret in a native app because client_secrets can't be reliably stored on devices
I thus would like to request to only send the client_secret, if one has been set.
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
So it does not get lost:
"Add code comment referring to rfc 6749 2.3.1. Client Password and how client_secret MAY be omitted."
Assignee | ||
Comment 2•5 years ago
|
||
Comment 3•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 4•5 years ago
|
||
Assignee | ||
Comment 5•5 years ago
|
||
Minor spelling error.
Assignee | ||
Updated•5 years ago
|
Comment 6•5 years ago
|
||
Assignee | ||
Comment 7•5 years ago
|
||
Scope is optional? Sure? How should that work with aAppKey not being optional behind it?
Comment 8•5 years ago
|
||
It's optional in regards to the RFC - https://tools.ietf.org/html/rfc6749#section-3.3.
Maybe you can also add that in the param comment: "as specified by RFC 6749 Section 3.3"
Internally Thunderbird will always have it set yes. But I think your use case was to use the module more widely, and then it's not required unless the OAuth2 provider requires/wants it.
Assignee | ||
Comment 9•5 years ago
|
||
Yes, found it in the spec just now. But for the javascript constructor we must make it required, but the caller may provide null to indicate that it should not be included, right? Just leaving the scope out (what [aScope] suggests to me) would map the aAppKey to the scope. If I am not missing the point here.
You do not want the JsDoc explenations alligned, but each with only a single space before and after the dash?
Comment 10•5 years ago
|
||
Ah, I see what you mean. It should indeed be (instead):
@param {?string} aScope - The scope as specified by RFC 6749 Section 3.3
You do not want the JsDoc explenations alligned, but each with only a single space before and after the dash?
Correct.
Assignee | ||
Comment 11•5 years ago
|
||
As I am not a native speaker, wording of comments and fitting everything within 80 chars per line is a PITA for me. If this is still not how you like it, I kindly ask you to drop them as comments in this bug and I will adjust the patch.
Assignee | ||
Updated•5 years ago
|
Comment 12•5 years ago
|
||
Comment 13•5 years ago
|
||
Assignee | ||
Comment 14•5 years ago
•
|
||
Now I know how you want the Js Docs to look like (and that makes it a lot easier) :-)
Comment 15•5 years ago
|
||
"falsy" is a typo - should be "false"? (I didn't examine the surrounding code)
Assignee | ||
Comment 16•5 years ago
|
||
Magnus cut that short. In my original draft I wrote "if it resolves to false (null, empty string, ...)". We do mean "falsy".
This is important as the client_secret behaves differently, it must be null to be excluded, but if it is an empty string, it will be included.
Comment 17•5 years ago
|
||
It's indeed intended to read falsy. Falsy is the term that covers null, undefined, "", 0 - stuff that evaluates to false in an if clause.
Comment 18•5 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/8c2f8de77106
allow callers to ommit sending OAuth2 client_secret parameter. r=mkmelin
Assignee | ||
Comment 19•5 years ago
|
||
Updated•5 years ago
|
Comment 20•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Comment 21•5 years ago
|
||
Would have to be uplifted with bug 1597933 which is more scary.
Comment 22•5 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #21)
Would have to be uplifted with bug 1597933 which is more scary.
Both have been on beta for a month and a half, so there's that. But a more cautious approach is fine - I'd suggest doing it in a 68.4.1 where we could do a slow rollout (which would be slightly problematic for a 68.4.0).
Comment 23•5 years ago
|
||
Comment 24•5 years ago
|
||
Comment 25•5 years ago
|
||
Description
•