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.

Sign up for GitHub

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

Jump to bottom

Add preliminary support for subscription keychain sharing #2538

Merged
merged 10 commits into from
Mar 6, 2024
Merged

Add preliminary support for subscription keychain sharing #2538

merged 10 commits into from
Mar 6, 2024

Conversation

quanganhdo
Copy link
Member

Task/Issue URL: https://app.asana.com/0/414235014887631/1206564116141608/f
Tech Design URL:
CC:

Description:

This PR does the followings:

  • Adding a new subscriptions app group
  • Injecting this app group into the subscriptions library to be used to create keychain entries.

Steps to test this PR:

It shouldn’t break any existing flow.

Copy Testing:

  • Use of correct apostrophes in new copy, ie ’ rather than '

Orientation Testing:

  • Portrait
  • Landscape

Device Testing:

  • iPhone SE (1st Gen)
  • iPhone 8
  • iPhone X
  • iPhone 14 Pro
  • iPad

OS Testing:

  • iOS 14
  • iOS 15
  • iOS 16

Theme Testing:

  • Light theme
  • Dark theme
Internal references:

Software Engineering Expectations
Technical Design Template

@quanganhdo quanganhdo requested a review from miasma13 March 5, 2024 14:40
This was referenced Mar 6, 2024
Merged
Merged
miasma13
miasma13 reviewed Mar 6, 2024
View reviewed changes
Copy link
Contributor

@miasma13 miasma13 left a comment

Choose a reason for hiding this comment

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

WDYT about using a pattern that @graeme used for macOS and from the providing a convenience initializer to the changes with the AccountManager on most places?

See https://github.com/duckduckgo/macos-browser/blob/6231a8e6c456c0f35a1498e2883ecc0d55d93b48/DuckDuckGo/Subscription/AccountManagerExtension.swift

quanganhdo
quanganhdo commented Mar 6, 2024
View reviewed changes
Copy link
Member Author

@quanganhdo quanganhdo left a comment

Choose a reason for hiding this comment

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

Updated to use convenience init.

DuckDuckGo/Subscription/UserScripts/SubscriptionPagesUseSubscriptionFeature.swift Outdated
@@ -319,7 +322,7 @@ final class SubscriptionPagesUseSubscriptionFeature: Subfeature, ObservableObjec
func restoreAccountFromAppStorePurchase() async throws {
setTransactionStatus(.restoring)

let result = await AppStoreRestoreFlow.restoreAccountFromPastPurchase()
let result = await AppStoreRestoreFlow.restoreAccountFromPastPurchase(appGroup: Bundle.main.appGroup(bundle: .subs))
Copy link
Member Author

Choose a reason for hiding this comment

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

I think the param name should be subscriptionAppGroup for consistency @miasma13.

@quanganhdo quanganhdo requested a review from miasma13 March 6, 2024 14:48
miasma13
miasma13 approved these changes Mar 6, 2024
View reviewed changes
Copy link
Contributor

@miasma13 miasma13 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

quanganhdo and others added 5 commits March 6, 2024 11:25
@quanganhdo
Merge branch 'main' into anh/subscription-token-sharing
892b3af 
# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
#	DuckDuckGo/SettingsViewModel.swift
#	DuckDuckGo/Subscription/UserScripts/SubscriptionPagesUseSubscriptionFeature.swift
@quanganhdo
Update BSK
e668813
@quanganhdo
Update BSK
a40f277
@quanganhdo
Fix SwiftLint
d93cebb
@graeme
Point to actual BSK release
816a52b
graeme added a commit to duckduckgo/macos-browser that referenced this pull request Mar 6, 2024
@graeme
Subscription keychain sharing for access token (#2292)
60403d5 
Task/Issue URL:
https://app.asana.com/0/414235014887631/1206564116141608/f
iOS PR: duckduckgo/iOS#2538
BSK PR: duckduckgo/BrowserServicesKit#690

**Optional**:

Tech Design URL:
https://app.asana.com/0/481882893211075/1206591576458107/f
CC: @quanganhdo @diegoreymendez 

**Description**:

For the macOS: Recheck entitlements and display error messaging for
cancelled subscriptions we need to make sure the VPN is no longer usable
once their entitlement to use it are withdrawn. In [iOS: Integrate with
Subscription](https://app.asana.com/0/0/1204886111391114), we’re already
checking the entitlements intermittently and on rekey, disabling the
VPN*.

To authenticate the VPN's register requests (to receive a server +
keypair for the tunnel configuration), the PacketTunnelProvider (central
component of the VPN extension) needs an access token which is to be
provided by the Subscription library built in O-1. This can be pulled
accessed directly from the library and, in the iOS and macOS AppStore
case, shared between processes (the app, the VPN agent, the VPN
extension) using a keychain group. We would like the fetching and
lifecycle to be managed exclusively by the Subscription library.

We will need to share / delete the subscription access token between
processes, so this task covers:
- Adding a new subscriptions app group
- Injecting this app group into the subscriptions library to be used to
create keychain entries.

**Steps to test this PR**:
1. Make sure subscriptions flow still works (don’t attempt to connect
VPN yet)
2. (Remaining steps in DEBUG only) Open the console and start logging
3. Connect to Network Protection.
4. After the macOS menu item has appeared, check the console and ensure
there is a log saying "VPN Agent found token"

---
###### Internal references:
[Pull Request Review
Checklist](https://app.asana.com/0/1202500774821704/1203764234894239/f)
[Software Engineering
Expectations](https://app.asana.com/0/59792373528535/199064865822552)
[Technical Design
Template](https://app.asana.com/0/59792373528535/184709971311943)
[Pull Request
Documentation](https://app.asana.com/0/1202500774821704/1204012835277482/f)
@graeme graeme merged commit 21ce049 into main Mar 6, 2024
13 checks passed
@graeme graeme deleted the anh/subscription-token-sharing branch March 6, 2024 20:15
samsymons added a commit that referenced this pull request Mar 6, 2024
@samsymons
Merge branch 'main' into sam/remove-cgnat
78f1beb 
# By Anh Do
# Via GitHub
* main:
  Add preliminary support for subscription keychain sharing (#2538)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
samsymons added a commit that referenced this pull request Mar 8, 2024
@samsymons
Merge branch 'main' into sam/vpn-ui-improvements
e2464c1 
* main:
  Point to BSK hotfix (#2558)
  Prevent redeeming invite codes after the VPN test ends (#2559)
  Updates Kingfisher to 7.11.0 (#2545)
  Makes network path metadata description anonymous (#2544)
  Adds pixels to track main VPN funnels (#2543)
  Cleanup after rolling out autoconsent enabled by default (#2537)
  Add #URL macro (#2540)
  Handle expired entitlement in NetP (#2525)
  Remove isSubscriptionEnabled check when attempting to delete NetP token (#2548)
  Validate correct environment (#2546)
  Remove CGNAT range (#2524)
  Add preliminary support for subscription keychain sharing (#2538)
  fix opening tabs with transitional (#2542)
  Integrate confirm entitlements endpoint (#2541)
  Makes dbSaveBloomFilterError daily and count (#2526)
  16. Subscription: Display "Activation in progress" message (#2535)
  Autofill support for deleting all passwords  (#2497)
  Bump BrowserServicesKit (#2532)
  Release 7.111.0-0 (#2534)
  if dax dialogs are showing then dismiss (#2506)
samsymons added a commit that referenced this pull request Mar 8, 2024
@samsymons
Merge branch 'sam/vpn-ui-improvements' into sam/vpn-ui-improvements-2…
112b37e 
…-lottie-animation

* sam/vpn-ui-improvements:
  Point to BSK hotfix (#2558)
  Prevent redeeming invite codes after the VPN test ends (#2559)
  Updates Kingfisher to 7.11.0 (#2545)
  Makes network path metadata description anonymous (#2544)
  Adds pixels to track main VPN funnels (#2543)
  Cleanup after rolling out autoconsent enabled by default (#2537)
  Add #URL macro (#2540)
  Handle expired entitlement in NetP (#2525)
  Remove isSubscriptionEnabled check when attempting to delete NetP token (#2548)
  Validate correct environment (#2546)
  Remove CGNAT range (#2524)
  Add preliminary support for subscription keychain sharing (#2538)
  fix opening tabs with transitional (#2542)
  Integrate confirm entitlements endpoint (#2541)
  Makes dbSaveBloomFilterError daily and count (#2526)
  16. Subscription: Display "Activation in progress" message (#2535)
  Autofill support for deleting all passwords  (#2497)
  Bump BrowserServicesKit (#2532)
  Release 7.111.0-0 (#2534)
  if dax dialogs are showing then dismiss (#2506)
samsymons added a commit that referenced this pull request Mar 8, 2024
@samsymons
Merge branch 'sam/vpn-ui-improvements-2-lottie-animation' into sam/vp…
70ec49d 
…n-ui-improvements-3-combine-notification-settings

* sam/vpn-ui-improvements-2-lottie-animation: (22 commits)
  Point to BSK hotfix (#2558)
  Prevent redeeming invite codes after the VPN test ends (#2559)
  Updates Kingfisher to 7.11.0 (#2545)
  Makes network path metadata description anonymous (#2544)
  Adds pixels to track main VPN funnels (#2543)
  Cleanup after rolling out autoconsent enabled by default (#2537)
  Add #URL macro (#2540)
  Handle expired entitlement in NetP (#2525)
  Remove isSubscriptionEnabled check when attempting to delete NetP token (#2548)
  Add dark mode animation.
  Allow the animation intro to be skipped
  Validate correct environment (#2546)
  Remove CGNAT range (#2524)
  Add preliminary support for subscription keychain sharing (#2538)
  fix opening tabs with transitional (#2542)
  Integrate confirm entitlements endpoint (#2541)
  Makes dbSaveBloomFilterError daily and count (#2526)
  16. Subscription: Display "Activation in progress" message (#2535)
  Autofill support for deleting all passwords  (#2497)
  Bump BrowserServicesKit (#2532)
  ...
samsymons added a commit that referenced this pull request Mar 8, 2024
@samsymons
Merge branch 'sam/vpn-ui-improvements-3-combine-notification-settings…
9f89201 
…' into sam/vpn-ui-improvements-4-location-setting-change

# By Sam Symons (4) and others
# Via Sam Symons (3) and others
* sam/vpn-ui-improvements-3-combine-notification-settings: (22 commits)
  Point to BSK hotfix (#2558)
  Prevent redeeming invite codes after the VPN test ends (#2559)
  Updates Kingfisher to 7.11.0 (#2545)
  Makes network path metadata description anonymous (#2544)
  Adds pixels to track main VPN funnels (#2543)
  Cleanup after rolling out autoconsent enabled by default (#2537)
  Add #URL macro (#2540)
  Handle expired entitlement in NetP (#2525)
  Remove isSubscriptionEnabled check when attempting to delete NetP token (#2548)
  Add dark mode animation.
  Allow the animation intro to be skipped
  Validate correct environment (#2546)
  Remove CGNAT range (#2524)
  Add preliminary support for subscription keychain sharing (#2538)
  fix opening tabs with transitional (#2542)
  Integrate confirm entitlements endpoint (#2541)
  Makes dbSaveBloomFilterError daily and count (#2526)
  16. Subscription: Display "Activation in progress" message (#2535)
  Autofill support for deleting all passwords  (#2497)
  Bump BrowserServicesKit (#2532)
  ...

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
samsymons added a commit that referenced this pull request Mar 11, 2024
@samsymons
Merge branch 'main' into sam/improve-vpn-metadata
b11d103 
* main: (28 commits)
  Fix VPN view model memory leak (#2570)
  Bump submodules/privacy-reference-tests from `40ce868` to `a603ff9` (#2500)
  18. Subscription Entitlements caching (#2556)
  Release 7.111.0-2 (#2563)
  Revert "Report Apple Ad attribution using pixel (#2510)" (#2562)
  Vanilla browser integration (#2550)
  Fix blank space after URL bar hides (#2549)
  Release 7.111.0-1 (#2561)
  Point to BSK hotfix (#2558)
  Prevent redeeming invite codes after the VPN test ends (#2559)
  Updates Kingfisher to 7.11.0 (#2545)
  Makes network path metadata description anonymous (#2544)
  Adds pixels to track main VPN funnels (#2543)
  Cleanup after rolling out autoconsent enabled by default (#2537)
  Add #URL macro (#2540)
  Handle expired entitlement in NetP (#2525)
  Remove isSubscriptionEnabled check when attempting to delete NetP token (#2548)
  Validate correct environment (#2546)
  Remove CGNAT range (#2524)
  Add preliminary support for subscription keychain sharing (#2538)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miasma13 miasma13 miasma13 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@quanganhdo @miasma13 @graeme