2050967 - [IPProtection] getAccessToken is getting called excessively

Status: RESOLVED FIXED

(Firefox for Android :: Privacy, defect)

Description

[Mike a [:mavduevskiy]]

Steps to reproduce

1. Create a new account, without giving it authorization to use VPN.
2. Log into that account.
3. Add logs/put debugger around getToken call.

Expected behavior

we don't get getToken request for a user that can not use the feature

Actual behavior

we do get that request, and multiple times upon authentication

Device information

• Firefox version:
• Android device model:
• Android OS version:

Any additional information?

Comment 1

[Pascal Chevrel (relman team) -> :pascalc]

S1 is incident-level and required an immediate unplanned dot release, hence the tracking flags automatically set for releases in flight. Downgrading to S2 as I don't think you intended to do that.
https://wiki.mozilla.org/BMO/UserGuide/BugFields#bug_severity:~:text=S1,available

Comment 2

[Mike a [:mavduevskiy]]

Attachment: WIP: Bug 2050967 - Don't init ipprotection module

... until user action or a successful authorization.

Comment 3

[Jonathan Almeida [:jonalmeida]]

Attachment: Bug 2050967 - Part 1: Add AccountScopeAccessor to read AccountStorage persisted state r=gmalekpour,markh

We introduce an accessor for reading our persisted account on disk by
consuming the JSON string first. This avoids having to know which
SharedPreference type is used (secure or regular).

The accessor acts as a weak implementation wrapper to a future API that
feeds in from the native fxa_client.

We always read the state from disk for every request to ensure
freshness. This uses a background Corountine context to avoid IO on
the FxaAccountManager that does not need to be synchronized with it's
own coroutineContext: this is a read-only check and reading may fail for
a variety of reasons, we do not need to block the account manager to
avoid these failures.

Comment 4

[Jonathan Almeida [:jonalmeida]]

Attachment: Bug 2050967 - Part 2: Introduce account scope check to IPProtection r=gmalekpour,markh

To avoid DDOS-ing our servers, we check our existing scopes to see if we
can fetch an access token. If we don't have one and we are
authenticated, we try to perform a token exchange if our
account does have the global scope.

This is similar to behaviour from before, except that we only attempt
the token exchange when we enter the VPN UI screen. If the account was
not entitled, we then perform an authorization as we previously did.

Co-authored-by: Mark Hammond <[e-mail]>

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e5344cff13c8
https://hg.mozilla.org/integration/autoland/rev/d4e6916363d2
Part 1: Add AccountScopeAccessor to read AccountStorage persisted state r=gmalekpour,markh,android-reviewers,mavduevskiy
https://github.com/mozilla-firefox/firefox/commit/e63cb7ea3102
https://hg.mozilla.org/integration/autoland/rev/a2d34562b7dd
Part 2: Introduce account scope check to IPProtection r=markh

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d4e6916363d2
https://hg.mozilla.org/mozilla-central/rev/a2d34562b7dd

Comment 7

[Jonathan Almeida [:jonalmeida]]

Attachment: Bug 2050967 - Part 1: Add AccountScopeAccessor to read AccountStorage persisted state

We introduce an accessor for reading our persisted account on disk by
consuming the JSON string first. This avoids having to know which
SharedPreference type is used (secure or regular).

The accessor acts as a weak implementation wrapper to a future API that
feeds in from the native fxa_client.

We always read the state from disk for every request to ensure
freshness. This uses a background Corountine context to avoid IO on
the FxaAccountManager that does not need to be synchronized with it's
own coroutineContext: this is a read-only check and reading may fail for
a variety of reasons, we do not need to block the account manager to
avoid these failures.

Original Revision: https://phabricator.services.mozilla.com/D309749

Comment 8

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: The VPN feature that is launching in 153 will make excessive requests to the FxA servers without these patches.
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: These patches will reduce the number of requests made to FxA as part of the VPN feature launch. The risk is low because if for some reason it doesn't do what it is intended, we are still in the same state of requests made.
• String changes made/needed?: n/a
• Is Android affected?: yes

Comment 9

[Jonathan Almeida [:jonalmeida]]

Attachment: Bug 2050967 - Part 2: Introduce account scope check to IPProtection

To avoid DDOS-ing our servers, we check our existing scopes to see if we
can fetch an access token. If we don't have one and we are
authenticated, we try to perform a token exchange if our
account does have the global scope.

This is similar to behaviour from before, except that we only attempt
the token exchange when we enter the VPN UI screen. If the account was
not entitled, we then perform an authorization as we previously did.

Co-authored-by: Mark Hammond <[e-mail]>

Original Revision: https://phabricator.services.mozilla.com/D309985

Comment 10

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/2f1c5b02f2cc
https://hg.mozilla.org/releases/mozilla-beta/rev/7b6817306fb8
https://github.com/mozilla-firefox/firefox/commit/ba5372c91e65
https://hg.mozilla.org/releases/mozilla-beta/rev/34c2fde2bd41

Comment 11

[Raluca Ilban [:rilban], Premium Services - VPN]

Is there something that QA can verify here? Thank you!

Comment 12

[Jonathan Almeida [:jonalmeida]]

(In reply to Raluca Ilban [:rilban], Premium Services - VPN from comment #11)

Is there something that QA can verify here? Thank you!

Thanks for checking in. No, I don't believe so. Engineering will need to monitor this dashboard to see the rejection numbers for vpn reduce to something reasonable.