This fixes a bug introduced in db655dd171 that could lead to a crash or
undefined behavior because it potentially updates resources (a
`@Published` object) from a background thread.
UI updates must occur on the main thread only.
The `load` and `save` functions in `Favorites` perform disk I/O and
should be wrapped with `withCheckedContinuation` in order to allow these
to be managed by the async runtime.
This was causing a detected hang on some devices with limited disk I/O
resources.
So it turns out we can get around the whole modal-blocking issue by
wrapping the `runModal()` call in `withCheckedContinuation`.
The key takeaway here is that `withCheckedContinuation` can be used to
wrap calls that can block I/O, marking them async, and allowing the Task
executor to do other useful work. This can even take place on on the
main thread.
Bear in mind `withCheckedContinuation` (and friends) *will not* prevent
blocking the task executor if the code being called is CPU-bottlenecked
in nature, such as calculating prime numbers or something. In those
cases, even `Task.detached` will cause Sentry to report `App Hanging`
alerts.
In short, only `Task.detached` when the I/O-related work to perform does
not depend on the current context and can be executed completely
independently of the parent, such as downloading image files in the
background or something.
`NSAlert`'s `runModal` method blocks the current thread until the user
takes action. When we do this via `await MainActor.run { ... }` from
within a `Task`, Sentry correctly detects this as blocking the executor
thread.
See https://github.com/getsentry/sentry-cocoa/issues/2643
As such, to show `NSAlert`s from within `Tasks` in Swift, we need to
wrap them with `withCheckedContinuation` to signal to the Task executor
we are yielding. Sentry won't complain if these are only blocking the UI
thread, which is unavoidable (and by design with `NSAlert`).
Since handling VPN status change events already occurs within a `Task`
in `VPNConfigurationManager` and we can't get around that, we mark the
handler called from there as `async` and add a fourth parameter, an
`NEProviderStopReason` which needs to be read from the tunnel process
via IPC when we encounter a `disconnected` state. This is needed because
the status change events we get from the system only include the status
and not the reason.
If the reason is `authenticationCanceled`, we run the modal.
This fixes a minor regression introduced in f779fe9667 where we would
incorrectly show an error dialog if the user cancels sign in because the
`authResponse` is invalid.
Adds SwiftLint using (mostly) default rules to our build pipeline.
Unfortunately, this results in quite a lot of errors / warnings.
All fixes included in this PR are simply refactoring. No functionality
has been changed.
Fixes: #4302
Under some conditions the call to SCDynamicStoreCreate can fail,
presumably due to some kind of allocation failure. Once it succeeds,
however, we can continue using the dynamic store for the lifetime of the
Adapter instance.
So we memoize the result of this call so as not to allocate each time we
need it.
See
https://developer.apple.com/documentation/systemconfiguration/1437828-scdynamicstorecreate
In the `didReceivePathUpdate` private function, we capture an implicit
hard reference to `self` because we access the `Adapter` instance
properties. This function is called in the workQueue by the
NWPathMonitor API and as such, we should weakly capture self throughout
to prevent a retain cycle.
To fix this, we use a `lazy var` for the `pathUpdateHandler` closure,
capturing `[weak self]` within it to use throughout the remainder of the
callback.
On iOS, it's possible for the notification granting process to throw
errors, though not very likely. This PR updates updates the plumbing for
how we request user notifications on iOS and respond to the result,
allowing for errors to be propagated back to the user in case something
goes wrong.
Note that unlike installing system extensions and VPN configurations,
disallowing notifications _does not_ result in an error being thrown.
Instead, we receive `false`, and we now track this Bool instead of the
entire `UNAuthorizationStatus` for updating the UI with.
By keeping that Bool `nil` until we load the authorization status, we
fix#7617 as a bonus.
Related: #7714Fixes: #7617
When processing the items in the Adapter's workQueue, it's possible the
Adapter has stopped by the time the queued closure begins execution.
This can possibly lead to obscure failures if for example we're trying
to apply network settings to a disconnected tunnel.
Supersedes: #7858
When installing the System Extension or VPN configuration, certain
errors can occur.
Some of these are due to user error and we should further advise the
user how to remedy.
For others, they aren't really actionable, and we silently ignore or
quietly log them.
Resolves: #7714
`fetchResources` is an IPC call. As such, it could potentially take a
long time to execute since the system may need to launch the XPC process
to handle the call.
Since this is called within a 1-second Timer whenever the user has the
MenuBar open (macOS) or is viewing the ResourcesList (iOS), we need to
run these IPC calls in a `Task.detached`.
The resources themselves must be updated on the main thread because
they're an `ObservableObject`, so a bit of refactoring is added to clean
this up a bit.
We are getting failures from `SCDynamicStoreCreate` and possibly will
also get failures from subsequent SystemConfiguration framework calls.
We can contextualize these errors with an error code retrieved from
`SCError()` which returns the error code from the most recent API call:
https://developer.apple.com/documentation/systemconfiguration/1516922-scerror
These can fail sporadically and we don't need to capture them. However,
for users who may be experiencing consistent failures or otherwise
wondering why their client isn't able to check for updates, we leave
them as `warning`.
On Apple, we will silently fail if the tunnel fails to start. This adds
a simple `NSAlert` modal (macOS) or `Alert` popup (iOS) that must be
dismissed before continuing if the tunnel fails to come up, so that the
user has a chance of understanding why.
The vast majority of the time this fails due to DNS lookup errors while
starting connlib.
Related: #7004
Supersedes: #7814
If the user again clicks `Enable System Extension` without having
actually enabled it in system settings, this error will be reported. We
don't necessarily need to act on it.
If the system extension has not been enabled by the user, opening the
`Settings -> Diagnostic Logs` pane will trigger this error. There's
nothing we can do until they enable the system extension, so don't
capture this particular case in Sentry.
When the app starts, we perform various checks in the
`AppViewModel.init` which read and write to disk, which can potentially
be slow (a few seconds), especially for busy rotational hard drives.
These were performed inside a regular `Task` closure, but since
AppViewModel is annotated `@MainActor`, that meant this Task blocked the
main UI thread until the operations completed.
In practice this wasn't an issue because it simply manifested as the app
taking a couple seconds to launch under these conditions.
We fix this by simply using a `Task.detached` which will run the
operations on another thread. Now, the first window will pop up sooner
and immediately show the `ProgressView()` (i.e. a loading spinner icon)
until these operations complete.
A few minor reorganizing of the `os()` macro was also performed because
some of the variables now need to be `await`ed because they live on the
main thread.
refs #7798
Opening URLs using `NSWorkspace.shared.open(url)` (which potentially
launches the browser) is a blocking operation on Apple platforms. This
will cause the UI to hang if called from a UI thread, so we need to
avoid that with a Task.
When starting a Task, by default it's launched with the same priority as
the calling code.
In the UI these are run on the `MainActor` with highest priority by
default. If the worker thread running the Task closure gets blocked, it
will cause the UI to hang.
To fix this, we use `Task.detached` which runs the closure without a
specific priority, which is lower than the UI thread.
Furthermore, `weak self` is used to prevent retain cycles if the parent
thread `deinit`s.
This was causing an issue primarily when making IPC calls because those
will sometimes hang until the XPC service is launched for the first
time.
---------
Signed-off-by: Jamil <jamilbk@users.noreply.github.com>
After reading through this [Apple technical
note](https://developer.apple.com/documentation/technotes/tn3137-on-mac-keychains),
it's clear that we want to actually omit this key from our keychain
queries.
The reason is because:
- on iOS, this will be already set (there is no other option)
- on macOS, the data protection keychain is *unavailable* from system
extensions
After testing, it appears that the original issue that PR sought to fix
was actually fixed by always installing the correct system extension
version: #7759.
Reverts firezone/firezone#7756
When building / testing the Apple clients locally, OS code signing and
security requirements can cause certain types of errors to throw.
We still want to see these in the console, but not necessary capture
them to Sentry.
When a user launches the macOS app, we check if the system extension is
installed. If it was, we assumed it would function properly.
However, an older version of the extension can be installed from our
current app version, so we would erroneously consider the extension as
"installed" even though it needed to be updated.
To fix this, we introduced an enum for tracking the system extension
state with `installed`, `needsReplacement`, and `needsInstall` states.
These track whether the extension is up-to-date, needs upgrade (or
downgrade), or needs to be approved and enabled by the user altogether
respectively.
Importantly, this also gracefully handles downgrades, not just upgrades
since we already return a `.replace` action in our request callback that
the system calls when installing an extension with the same bundle ID as
one that exists.
This will force the macOS Keychain to behave like the iOS Keychain. To
be honest, Apple's documentation is very much lacking in this regard,
but some research suggests this is both heavily recommended by Apple and
that it enables the Keychain operation to benefit from Apple's security
hardware in their Macs.
In my local testing, it also seems to make keychain operations more
reliable when SIP is disabled, but that could be a fluke given the
number of variables at play.
https://developer.apple.com/documentation/security/ksecusedataprotectionkeychain
Draft because stacked.
This function is called from `PacketTunnelProvider.startTunnel`, which
already uses the `completionHandler` approach for returning to the
caller when the tunnel start operation is completed.
Thus `async / await` here is redundant and unnecessary.
The Keychain on Apple platforms, while secure, is not always available.
It can be unavailable if the user has changed its permissions
accidentally, the keychain database is corrupt, there is an issue with
the secure enclave, or any number of other system-related or Apple
account-related reasons.
There are only two things we use the Keychain for:
- Storing the `firezone-id`. This is actually not a secret.
- Persisting the `token` upon sign in so that:
- the iOS system can keep the tunnel alive without the GUI running
- the macOS app can relaunch after `Disconnect & Quit` without having to
sign in again
For the first case, we move back to persisting this to a file (see
#7464).
For the second case, we simply don't care too much if the Keychain can't
be saved to. We simply move on with activating the tunnel and logging
the error so we know how often these edge cases occur.
On macOS 12, returning an empty body for a `WindowGroup` can cause the
app UI to crash with the following error:
```
*** Assertion failure in void _NSWindowSetFrameIvar(NSWindow *, NSRect)(), NSWindow.m:935
```
Since the `menuBar` is not initialized when the app initializes, this
conditional can return an empty body in a race condition.
Even though we're winding down support for macOS 12, it would be good to
fix this logic bug.
Xcode doesn't allow wildcards in input file lists, so the rules I set up
in #7488 never took effect.
Upon further investigation, it appears that the `strip` command executed
unconditionally at the end of every Rust build was the culprit. Since
Xcode already does this for us, it's a useless step that adds about 30s
to the build time.
Unfortunately there isn't a good way to tell Xcode not to build rust.
But now we don't need to -- `cargo`'s build cache is smart enough to
skip builds and we are back to the ~1-2s range for repeated builds when
only Swift code has changed.
We also add the swift bridge generated code to version control. These
doesn't change regularly, and Xcode sometimes complains that the files
don't exist _before_ it lets you run the `cargo build` to generate them
🙃 .
For a while now, `connlib` has been calling these two callbacks right
after each other because the internal event already bundles all the
information about the TUN device. With this PR, we merge the two
callback functions also in layers above `connlib` itself.
Resolves: #6182.
This code doesn't make sense:
- In the Adapter, we are not running on the main UI thread
- In this callback, we are running on the `workQueue` anyway
- `Task` is not how to spawn a new thread in Swift strictly speaking
In #7680, I stated that a VPN profile's status goes to `.invalid` when
its associated system extension is yanked. That's not always true. I
observed cases where the profile was still in the `.disconnected` state
with the system extension disabled or removed.
So, now we explicitly check on startup for two distinct states:
- whether the system extension is installed
- whether the VPN profile is valid
Based on this, we show an improved GrantVPNView for macOS only with
clear steps the user must perform to get set up.
If the user accidentally closes this window, they can open the Menubar
and click "Allow VPN permission to sign in" which will both (re)install
the extension and allow the profile.
<img width="1012" alt="Screenshot 2025-01-07 at 5 23 19 PM"
src="https://github.com/user-attachments/assets/c36b078e-835b-4c6e-a186-bc2e5fef7799"
/>
<img width="1012" alt="Screenshot 2025-01-07 at 5 24 06 PM"
src="https://github.com/user-attachments/assets/23d84af4-4fdb-4f03-b8f9-07a1e09da891"
/>
<img width="1012" alt="Screenshot 2025-01-07 at 5 31 41 PM"
src="https://github.com/user-attachments/assets/5b88dfa4-1725-45f2-bd6e-1939b5639cf4"
/>
On macOS, there are two steps the user needs to allow for the VPN
profile to be active: enable the system extension and allow the VPN
profile.
The system extension must be installed before the VPN profile can be
allowed. This PR updates the flow so that the user is prompted to handle
both of those serially. Before, we tried to install the system extension
on launch and prompt the user to allow the profile at the same time.
This PR includes fixes to handle the edge cases where the user removes
the profile and/or extension while the tunnel is connected. When that
happens, the `NEVPNStatus` becomes `.invalid` and we replace the `Sign
In` link in the Menubar with a prompt to restart the grant permissions
flow. On iOS, the behavior is similar -- we move the view back to the
GrantPermissionsView.
Lastly, some refactoring is included to use consistent naming for
`VPNProfile` instead of `TunnelManager` to more closely match what Apple
calls these things.
This is a very minor regression caused by #7555. We didn't bind the
status update observer until after the VPN profile was created. In
practice this didn't cause an issue because the status never changes
until a VPN profile is created, but still thought it's good to fix.
The JSON encoder is now configured in the `init()` as well like any
other instance variable should be.
If the user removes a VPN profile while it's connected, and then re-adds
a fresh one, the system will bring the tunnel back up with default
configuration, which means a blank `accountSlug`.
Instead of crashing because this isn't set, we bail the tunnel connect
with an error.
Although interacting with the Keychain typically involves I/O, there
wasn't a good reason to make all of its functions `async` as this
infects the caller hierarchy. Instead, the calling code should decide
how to wrap the synchronous calls directly with their own preferred
mechanism for dealing with blocking operations.
In practice, it turns out that nearly all of the places we were
interacting with the Keychain was _already_ within some sort of
non-critical Task or equivalent, especially the important parts like the
UI thread. So this complexity is not required.
The Keychain was moved from an `actor` class to a regular enum because
we don't need the serialization guarantees here -- since the calls are
blocking, there is no risk of Keychain operations happening out of
order.
Tested on macOS 15.
