-
Notifications
You must be signed in to change notification settings - Fork 212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Add oauth_webchannel_v1 for Fenix and GeckoView apps #2043
Conversation
detail = JSON.parse(event.detail); | ||
} catch (e) { | ||
detail = event.detail; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why this change was necessary, is it a limitation of the webextension webchannel receiver?
I'd also be tempted to do a typeof event.details
here to ensure you don't try to JSON.parse
something that's not a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the API of the WebChannel message change with Fenix/GeckoView?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WebExtensions in Geckoview/ Fenix cannot pass js objects, only strings. Otherwise it displays [protected]
, [inaccessible]
or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, I also recall an ancient bug about sending only strings on Desktop as well, which has had to get added to a special allowlist in order to bypass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's when sending an event to the browser that detail has to be stringified:
@vladikoff is there a train you want to land this in and estimate of work? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach seems reasonable, the amount of duplicate code in oauth-webchannel-v1 is pretty hefty. Could the OAuthWebChannelBroker's prototype be extended with the duplicate methods from the OAuth broker, or maybe a Cocktail mixin to share between the two?
What would be very useful is a short doc how the protocol works from an integration P.O.V., e.g., how to open FxA, what messages to listen for, and what should happen when those messages come in. Having that recorded will be useful to point people at (ourselves included) as an explanation for how it all works.
detail = JSON.parse(event.detail); | ||
} catch (e) { | ||
detail = event.detail; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the API of the WebChannel message change with Fenix/GeckoView?
* @returns {Promise} Returns a promise that resolves into an encrypted bundle | ||
* @private | ||
*/ | ||
_provisionScopedKeys(account) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a ton of code duplicated between here an the oauth-redirect broker, can this be shared somehow?
{ | ||
"code": "02f3cfea84ac4c143662b38d6c7f0c82c6f91eb041befc7cecda446b1b4887c1", | ||
"state": "vHao1p6OizzwReCkQMSpZA", | ||
"redirect": "http://127.0.0.1:3030/oauth/success/a2270f727f45f648?code=02f3cfea84ac4c143662b38d6c7f0c82c6f91eb041befc7cecda446b1b4887c1&state=vHao1p6OizzwReCkQMSpZA", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this right, or would it be an urn like urn:ietf:wg:oauth:2.0:oob:oauth-redirect-webchannel
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes, we can do that!
Continued in #2320 |
Fixes #1980
Ref: mozilla-mobile/android-components#3881