KM-17461: decouple VPN permission grant from server list availability

[kp-nils-crefeld]

Summary

After a fresh signup the server list is empty (logout wipes the cache and the re-download is daemon-driven), so install() could not build a profile (noServersAvailable) and the permission screen looped an undismissable "We need this permission" alert.

• Add VPNProvider.obtainVPNPermission(): saves the profile with a placeholder server when no target server is available yet, so the one-time OS VPN permission no longer depends on the server list. connect() already re-resolves and re-saves the real server before starting the tunnel.
• VPNPermissionViewController: use obtainVPNPermission() and stop auto-retrying from the alert's OK action (the loop mechanism).
• PIATunnelProfile: fall back to the server hostname instead of persisting an empty serverAddress when no OpenVPN address resolves.
• Add VPNPermissionPlaceholderTests locking the non-empty serverAddress invariant for WireGuard and OpenVPN.

[claude]

Claude finished @kp-nils-crefeld's task in 7m 35s —— View job

---

Code review: KM-17461 — decouple VPN permission grant from server list

• Read the PR diff (git diff origin/master...HEAD)
• Review DefaultVPNProvider.obtainVPNPermission() / vpnClientConfiguration(allowServerPlaceholder:)
• Review Server+PermissionPlaceholder and the PIATunnelProfile hostname fallback
• Review VPNPermissionViewController alert-loop fix
• Assess test coverage & impacted flows
• Post review (6 inline comments + summary below)

---

Overall

The diagnosis is right and the fix is well-scoped: the OS VPN permission is a one-time grant that has no business depending on a downloaded server list, and connect() genuinely does re-resolve targetServer and re-save before starting the tunnel (DefaultVPNProvider.swift:247-268), so the placeholder never gets dialed on the happy path. Removing the submit() re-entry from the alert's OK action kills the actual loop mechanism rather than papering over it. Log messages, doc comments and the // KM-17461 breadcrumbs are all good.

I filed 6 inline comments. The ones I'd want addressed before merge:

1. Server.vpnPermissionPlaceholder is a shared mutable global — Server is a non-Sendable public final class with var isAutomatic. A computed static var gives a fresh instance per access with zero call-site churn. (inline)

2. Placeholder hostname is inside the real PIA domain — placeholder.privateinternetaccess.com can resolve, and it matches the substring test in needsMigrationToGEN4() (DefaultVPNProvider.swift:466-478). An RFC 6761 .invalid name can't do either. (inline)

3. Test file will not compile for tvOS — PIATunnelProfile/PIAWGTunnelProfile live behind #if os(iOS) and the package declares .tvOS(.v17). MockProviders.swift:31 already guards for this. It passes in PIA-VPN-iOS.xctestplan (where PIALibraryTests is included and these tests are not in the skip list, so they will run), but swift test/a tvOS destination breaks. (inline)

4. On-demand on the placeholder profile — the placeholder configuration still inherits isOnDemand: isPersistentConnection. For a fresh manager doSave's ternary lands on false, so it's inert as documented; but if the screen is reached while an already-enabled manager exists, you save on-demand rules pointing at a bogus endpoint and the OS can try to bring it up. Forcing isOnDemand: false for the placeholder path removes the whole class of concern. (inline)

Two things outside the diff

tvOS has the identical bug and isn't fixed. PIA VPN-tvOS/VPNConfigurationInstalling/Data/InstallVpnConfigurationProvider.swift:23 grants permission with the same vpnProvider.install(force: true) and maps any error to InstallVPNConfigurationError.userCanceled — so a fresh tvOS signup with an empty server list tells the user they cancelled. Switching that use case (and VpnConfigurationProviderType) to obtainVPNPermission() looks like a small follow-up; intentional to defer?

The first connect() after this still dead-ends. The permission screen now dismisses, but pressing Connect before ServersDaemon finishes returns vpnClientConfigurationUnavailable — same empty-list root cause, one screen later. Note Client.bootstrap() already preloads configuration.bundledServersJSON, and loadLocalJSON only fills when currentServers.isEmpty (DefaultServerProvider.swift:146) — re-loading the bundled list after logout would fix the permission screen and the first connect and tvOS, from one place. Worth considering as the general fix (or a follow-up ticket) with this PR kept as defence in depth.

Test coverage

The two added tests lock a real invariant, but they're the least valuable of the available options: WireGuard already fell back to the hostname before this PR, and customConfiguration: nil makes the OpenVPN test skip the OpenVPNProvider.Configuration branch that actually fails in production. The genuinely new logic — the placeholder-vs-nil branch in vpnClientConfiguration — has no test because the method is private; making it internal would let @testable tests assert both halves (install(force:) still fails without servers, obtainVPNPermission() succeeds with the placeholder). Also no tearDown restoring lastServerCN, which both generatedProtocol paths write.

Memory / threading

No retain cycles introduced. The submit() closure captures self strongly and performs present/dismiss from a saveToPreferences completion (undocumented queue) or, on the early-error paths, synchronously — worth an explicit main-queue hop and [weak self] (inline). No new shared-state mutation beyond the static let noted above.

Flows to test

• Fresh signup → permission screen (the reported bug), on WireGuard and OpenVPN and IKEv2 — permission granted, screen dismisses, no alert loop.
• Genuine denial: tap OK, then hit Cancel on the iOS system permission sheet → alert appears once and is dismissable; the screen's OK button retries successfully.
• First connect immediately after granting, before the server list lands — confirm the UX is acceptable (see above).
• Existing-profile paths: protocol switch (WG ↔ OpenVPN ↔ IKEv2) and Settings-level "Delete VPN configuration" followed by re-grant — verify no placeholder profile is left behind in Settings › VPN and that on-demand/NMT rules aren't attached to it.
• NMT / always-on enabled while granting permission.
• tvOS onboarding after fresh signup — expected to still fail; confirm scope.
• Regression: OpenVPN over TCP against a region with no TCP address (the other path the serverAddress fallback now changes).

Verification I could not run

This runner is Linux with no Xcode/simulator, so I did not build the project, run bundle exec fastlane iOStests, or run swift format over the changed files. Findings are from source reading only; the tvOS-compilation point (#3) in particular is worth confirming with a local build.
• branch KM-17461-vpn-permission-placeholder

[claude]

Claude finished @kp-nils-crefeld's task in 3m 52s —— View job

---

Follow-up review: commit 61c5ffd9 (review feedback)

• Read the incremental diff (b2db530c..61c5ffd9)
• Verify each of the 6 earlier findings
• Check for new issues introduced by the fixes
• Post verdict

---

All four blocking items are correctly addressed

#	Finding	Status
1	Shared mutable static let placeholder	✅ now a computed static var — fresh instance per access, no call-site churn
1b	Hostname inside the PIA domain	✅ vpn-permission-placeholder.invalid — RFC 6761, cannot resolve, and no longer matches the address.contains("privateinternetaccess.com") test in needsMigrationToGEN4()
2	On-demand on the placeholder profile	✅ isOnDemand: usesServerPlaceholder ? false : accessedPreferences.isPersistentConnection (DefaultVPNProvider.swift:453)
3	Test file wouldn't compile for tvOS	✅ #if os(iOS) guard; indentation matches .swift-format's indentConditionalCompilationBlocks: true, imports satisfy OrderedImports
4	OpenVPN test bypassed the real branch	✅ now builds a production-like OpenVPNProvider.Configuration
5	lastServerCN not restored	✅ saved in setUp, restored in tearDown (it's String?, so the optional restore compiles)
6	Callback queue + strong self in the VC	✅ [weak self] + explicit DispatchQueue.main.async

I traced the OpenVPN test to confirm it now walks the branch it claims: endpointProtocols contains a TCP entry → PIATunnelProfile.swift:299 takes the TCP path → bestAddressForOVPN(tcp: true) returns nil on the empty-address placeholder (Server.swift:292-294) → serverAddress stays "" → the new hostname fallback fires. That's faithful to the production failure now.

One new caveat from the on-demand fix (non-blocking)

RootCoordinator.handleAuthenticationSuccess() presents the permission screen after login as well as signup (RootCoordinator.swift:163-171), unconditionally — there's no check for permission already being granted. So for a returning user who logs back in while the server cache is empty:

• doSave evaluates vpn.isOnDemandEnabled = isVPNConnected || vpn.isEnabled ? configuration.isOnDemand : false (NetworkExtensionProfile.swift:72). With an already-enabled manager and the placeholder's isOnDemand: false, this clears their on-demand rules while Client.preferences.isPersistentConnection stays true → system state and preference diverge until the next connect()/save re-writes it.
• The saved profile also shows vpn-permission-placeholder.invalid in Settings › VPN until the first connect (cosmetic, but worth eyeballing on device).

This isn't a regression — pre-PR that path errored out entirely — and it self-heals on the next connect. If you want it fully closed, the cheapest options are to skip the save when the OS permission already exists, or to preserve the manager's current isOnDemandEnabled on the placeholder path instead of forcing false. Your call whether that's worth it versus the autoconnect hazard the false closes. Fix this →

Minor nit

Package.swift declares TunnelKitOpenVPN for the test target, but the test also does import TunnelKitCore, which isn't declared. It compiles — it resolves transitively, exactly as VPNDaemon.swift and PIATunnelProvider+Profile.swift already do in Sources without declaring it — so this is consistency, not a build break. Declaring it alongside (same .when(platforms:) condition) would be more honest.

Still open by choice — fine to defer, just confirming scope

• obtainVPNPermission remains a bare public protocol requirement (VPNProvider.swift:68) with no default implementation — source-breaking for out-of-repo conformers only.
• vpnClientConfiguration(for:allowServerPlaceholder:) is still private, so the placeholder-vs-nil branch itself has no direct test (the tests cover the profile-generation invariant instead).
• tvOS still has the same bug: InstallVpnConfigurationProvider.swift:22 calls install(force: true) and maps any error to .userCanceled.
• PIAWGTunnelProfile.swift:294 still writes lastServerCN unconditionally, so a WireGuard permission-only save clobbers a previous CN with "" (pre-existing, low impact).
• The first connect() before ServersDaemon lands still returns vpnClientConfigurationUnavailable — same root cause, one screen later. Re-loading configuration.bundledServersJSON after logout remains the broader fix if you want a follow-up ticket.

Verification I could not run

Linux runner with no Xcode/simulator: no xcodebuild, no bundle exec fastlane iOStests, and SPM dependencies use git@github.com: SSH remotes so I couldn't resolve/build the package either. swift format is present on the runner but running it wasn't permitted in this job, so formatting is eyeballed against .swift-format, not verified. The tvOS-compilation fix (#3) and the new TunnelKitOpenVPN test dependency are the two things most worth confirming with a local build before merge.

Verdict: the review feedback is addressed correctly and the changes hold up on re-reading. Nothing blocking from my side.
• branch KM-17461-vpn-permission-placeholder

[kp-mario-nachbaur]

LGTM