fxa-client: Don't expose the session token directly to consumers.#7317
fxa-client: Don't expose the session token directly to consumers.#7317mhammond wants to merge 1 commit intomozilla:mainfrom
Conversation
mhammond
force-pushed
the
push-yqytmmnsxvyx
branch
2 times, most recently
from
e729e18 to
b80e90d
Compare
bendk
left a comment
bendk
left a comment
There was a problem hiding this comment.
I like this change. The more we can move the logic into the component the better, IMO. That way if there's a bug or change we only need to change the Rust code.
| /// The JSON payload is the `data` object from the `fxaccounts:login` WebChannel command. | ||
| pub fn handle_web_channel_login(&mut self, json_payload: &str) -> Result<()> { | ||
| let data: serde_json::Value = serde_json::from_str(json_payload)?; | ||
| if let Some(token) = data.get("sessionToken").and_then(|v| v.as_str()) { |
There was a problem hiding this comment.
Is there ever a scenario where we wouldn't get the sessionToken? I see NoSessionToken below for pw change but here we still return success even if no sessionToken?
There was a problem hiding this comment.
yeah, good spot thanks, this should probably return the same error.
skhamis
left a comment
skhamis
left a comment
There was a problem hiding this comment.
I agree I think this is an awesome improvement! Love to see it!
mhammond
force-pushed
the
push-yqytmmnsxvyx
branch
from
b80e90d to
93fbc2f
Compare
This was exposed primarily for use with the web channel, but the fact it is exposed means that consumers are free to use it in ways we'd like to better control. We do this by adding a couple of new methods which are explicitly used only for webchannels, and the data moved over those APIs are abstracted away - the consumers just pass the raw JSON data from the webchannel message and the component knows what format it is in and decodes it appropriately. This is a breaking change for Android and iOS: TODO: link to PRs for them.
mhammond
force-pushed
the
push-yqytmmnsxvyx
branch
from
93fbc2f to
2926443
Compare
|
This is a good step! I think protecting against using the session token incorrectly is more valuable than protecting against holding it - the |
4 participants
This was exposed primarily for use with the web channel, but the fact it is exposed means that consumers are free to use it in ways we'd like to better control. We do this by adding a couple of new methods which are explicitly used only for webchannels, and the data moved over those APIs are abstracted away - the consumers just pass the raw JSON data from the webchannel message and the component knows what format it is in and decodes it appropriately.
This is a breaking change for Android and iOS:
@jonalmeida @bendk @skhamis @LZoog - I think this is a good improvement, wdyt?
Pull Request checklist
[ci full]to the PR title.