mergesort / Boutique

✨ A magical persistence library (and so much more) for state-driven iOS and Mac apps ✨
https://build.ms/boutique/docs
MIT License
920 stars 45 forks source link

Error updating @SecurelyStoredValue #61

Closed otymartin closed 5 months ago

otymartin commented 6 months ago

When updating my currentUser

@SecurelyStoredValue<CurrentUser>(key: "currentUser")
var currentUser: CurrentUser?

I get the following error.

try await $currentUser.set(remoteCurrentUser)

/// errorWithStatus(status: -25303)

Not sure if I need a keychain entitlement of some kind 🤔

mergesort commented 6 months ago

Hey @otymartin, so sorry about this bug. I just came across it a few days ago but haven't had a chance to delve too deeply into a solid fix, but I do have a workaround you can use in the mean time.

It's ugly but if you need this to work right now you can set the value to nil before setting it to the value you need. That will remove the value from the keychain and then set will go down the path of inserting a new value (which works), and will never hit the update path which has a permissions-related bug I don't yet understand in it.

The code would look something like this

try await $currentUser.set(nil)
try await $currentUser.set(remoteCurrentUser)

Or if you'd like to a slightly cleaner more generic version, you can choose between one of these two options.

// Working around a Boutique bug where the update branch of setting a keychain value is not working.
// This will be fixed in the future, and when it is this code should be removed.
extension SecurelyStoredValue {
    @MainActor
    public func forceSet(_ value: Item?) throws {
        try self.set(nil)
        try self.set(value)
    }

    @MainActor
    public func set(_ value: Item?, forceResetBeforeSettingValue: Bool) throws {
        if forceResetBeforeSettingValue {
            try self.set(nil)
        }

        try self.set(value)
    }
}

I would recommend adding either of those functions into an extension so it's easier to remove in the future when the bug is fixed by searching for forceSet or forceResetBeforeSettingValue and going back to using the regular version of set.

mergesort commented 5 months ago

I've updated set to the equivalent of forceSet above on the securely-stored-value-set-fix branch to ensure users have a means to achieve the correct functionality.

I don't want to push it to the main branch yet because I'm unsure if this will cause any performance issues (though I don't think it should in most use cases), so I'm going to leave this issue open because I want to make sure I properly fix this issue in the future.

mergesort commented 5 months ago

I didn't witness any performance issues so I've merged this fix into main, released in version 2.4.4. Let me know if you run into any issues!