Skip to content
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

Display error messaging for cancelled subscriptions #2394

Merged
merged 115 commits into from
Mar 19, 2024

Conversation

graeme
Copy link
Collaborator

@graeme graeme commented Mar 12, 2024

Task/Issue URL: https://app.asana.com/0/72649045549333/1205438842252963/f

Description:

When we detect that entitlements have expire AND Netp is enabled:

  • Disable NetP
  • Show revoked notification and make sure that Netp disabled notification is NOT shown.
  • On opening the status view we show the revoked dialog.
  • When we get 403 response from /register (or any controller APIs), we handle the expired entitlement (refer to first bullet point in this section). (Handled by the iOS equivalent project
  • When rekey is attempted with invalid entitlements, we will get 403. (Handled by the iOS equivalent project
  • If user attempts to enable Netp with invalid entitlements, we will get 403. (Handled by the iOS equivalent project
    We do a periodic check every 20 minutes calling the subscription API to get entitlements (Handled by the NetworkProtectionEntitlementsMonitor implemented in the iOS equivalent project )
  • If check succeeds and user has entitlement, we do nothing.
  • If check succeeds and user has no entitlement, we handle the expired entitlement (refer to first bullet point in this section).
  • If check fails (no internet, BE failure, tunnel failure), we do nothing.
  • If Netp is disabled, we stop this check.

On top of this, we also added an additional check on the Settings page.

  • To correctly show the Netp entry, we check for entitlement through subscription API whenever the Settings page is foregrounded.
  • If check succeeds and user has entitlement, we show the Netp settings entry.
  • If check succeeds and user has no entitlement, we handle the expired entitlement (refer to first bullet point in this section) and also hide Netp settings entry.
  • If check fails (no internet, be failure, tunnel failure), we retain whatever the previous visibility of the Settings entry if available, else hide entry.

Steps to test this PR:
https://app.asana.com/0/0/1206779809102187/f


Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

@@ -47,6 +47,10 @@ final class DuckDuckGoVPNApplication: NSApplication {

super.init()
self.delegate = _delegate
#if SUBSCRIPTION
SubscriptionPurchaseEnvironment.currentServiceEnvironment = .staging
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be configured by settings.environment?

await entitlementMonitor.start(entitlementCheck: entitlementsCheck) { [weak self] result in
switch result {
case .validEntitlement:
UserDefaults.netP.networkProtectionEntitlementsExpired = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again wondering if we can check for entitlements instead of relying on a shared user default.

func requireAuthTokenOrKillApp() {
func requireAuthenticationOrKillApp() {
#if SUBSCRIPTION
Task {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the code in the bouncer is async, we need the full method to be async.

This call needs to be blocking for the other logic that happens after in the caller, so that the tunnel won't try to start while this is executing. I'm making a similar change that you can see here.

@diegoreymendez diegoreymendez self-requested a review March 18, 2024 14:03
Copy link
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one blocking issue: it seems geolocation is broken at least for waitlist users. I think it may have to do with the environment being hard-coded to staged, but I'm not sure.

@diegoreymendez diegoreymendez self-requested a review March 19, 2024 13:10
Copy link
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've reviewed this several times in the code, in testing and live on a call.

Approved.

@samsymons samsymons merged commit cf1e711 into main Mar 19, 2024
17 checks passed
@samsymons samsymons deleted the graeme/entitlements-change-handling-spike branch March 19, 2024 18:52
samsymons added a commit that referenced this pull request Mar 19, 2024
* main:
  Display error messaging for cancelled subscriptions (#2394)
  Updated Settings Page (#2329)
  Add VPN & PIR thank you modal (#2437)
  Web pixels (handlers + pixels) (#2451)
  Subscription design review further fixes (#2448)
  Address bookmarks feedback (#2411)
  Handle subscription-related iOS use cases (#2427)
samsymons added a commit that referenced this pull request Mar 20, 2024
# By Michal Smaga (2) and others
# Via GitHub (2) and Dominik Kapusta (1)
* main:
  Connect thank you message to DBP method (#2456)
  Use SubscriptionFeatureAvailability to determine availability of the subscription (#2436)
  Add Privacy Pro to App Store build (#2440)
  Display error messaging for cancelled subscriptions (#2394)
  Updated Settings Page (#2329)
  Add VPN & PIR thank you modal (#2437)
  Web pixels (handlers + pixels) (#2451)
  Subscription design review further fixes (#2448)
  Address bookmarks feedback (#2411)
  Handle subscription-related iOS use cases (#2427)
  Remove hardcoded NetP staging endpoint (#2446)
  DBP: Make webview non-persistent and delete any old cache data (#2445)
  Prevents the tunnel from starting without an auth token (#2438)
  Use History in Suggestions on iOS (#2339)
  When publishing a DMG, only check out the branch if it exists, otherwise stay on main
  Bump version to 1.80.0 (146)
  Update autoconsent to v10.3.0 (#2433)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
samsymons added a commit that referenced this pull request Mar 21, 2024
# By Diego Rey Mendez (4) and others
# Via GitHub (2) and Dominik Kapusta (1)
* main: (28 commits)
  Entitlements cache fixes (#2465)
  BSK update for RMF app build version improvement (#2362)
  Surface the last-used password first in autofill prompt when filling passwords (#2431)
  Reset subs VPN state from debug menu  (#2462)
  iOS UI improvements (#2412)
  Updates VPN 128 assets (#2459)
  Fix sparkle strings for thank you message (#2461)
  Wire VPN environment to subs menu item  (#2460)
  Makes a method thread safe (#2452)
  Ensure smooth subs updates (#2424)
  Update Pointfree snapshot testing library to 1.15.4 (#2449)
  Connect thank you message to DBP method (#2456)
  Use SubscriptionFeatureAvailability to determine availability of the subscription (#2436)
  Add Privacy Pro to App Store build (#2440)
  Display error messaging for cancelled subscriptions (#2394)
  Updated Settings Page (#2329)
  Add VPN & PIR thank you modal (#2437)
  Web pixels (handlers + pixels) (#2451)
  Subscription design review further fixes (#2448)
  Address bookmarks feedback (#2411)
  ...

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants