Cannot update an existing record in the keychain

[skington]

This isn't actually a pull request, as (a) there's a lot of code duplication in the tests, and (b) the tests currently fail! But it was the easiest way that occurred to me of bundling up the code I have currently that's failing.

It naïvely appears to be a recurrence of the very first issue in this project, but I have absolutely no experience in debugging Swift so I couldn't say.

For a Secret Work Project, we need an app that logs into our web service, and we need to remember those login details. So GenericPasswordSecureStorable looked like the easiest way of implementing this, albeit with a constant account value as we'd only ever store one set of user details.

As you'll hopefully see when running the tests, the C, R and D part of CRUD work fine; but when I try to update the details, I get an exception. And I get that whether I update the original object or I try to store a new one (which is good - if it had varied between which object was trying to store stuff that would have been the stuff of nightmares).

I'm new to Swift (normally I'm a Perl programmer), so some of the code might be unidiomatic, and hopefully the fix is to say "why on Earth are you doing that‽", update the documentation, and tell me not to be such a fool. But in case it actually is an issue in the guts of something horrible, you have a test method to run now.

[matthewpalmer]

❤️❤️❤️ love a bug report with a good test case—thanks!

These are the failures I'm getting here—let me know if you're seeing something different

I might be misinterpreting here, but I think the library is behaving as intended in this case (though maybe how it works should change...). When secondUser.store() is called, in ends up calling createInSecureStore which won't replace the existing item in the keychain, and instead will throw an error. (.Duplicate, which leads to the logging of Couldn't store: Duplicate.)

The keychain just looks uses the values of the account/service pair to determine what's unique or already stored, so changing the username in this instance won't cause a new account to be stored (since, as you say, the account is constant).

It's possible to extend CreateableSecureStorable with an update method (or you can put this on your User type if that's preferred):

extension CreateableSecureStorable where Self : DeleteableSecureStorable {
    func replaceData() throws {
        do { try self.deleteFromSecureStore() } catch {} // Ignore any errors
        try self.createInSecureStore()
    }
}

Let me know if I've misunderstood/misinterpreted and I can take another look :)

Side note (would love to hear your thoughts on this, but feel free to skip if you don't have time to answer):

You are definitely not the first person who has expected that the create method would overwrite any existing data. The decision not to overwrite was made in an earlier incarnation of the library just never changed, though I'm not against changing it if there are considerable numbers of people confused by it. Although maybe this is just a documentation problem. I guess I have a couple of questions:

• do you think the create method should overwrite or does the documentation just need to be improved?
• how would you improve the documentation?

I'm super interested in the second point because you're a new Swift dev and I want to try to make Swift stuff as accessible to everyone as possible—what have you found confusing about Swift, Locksmith and its documentation in general?

(Also worth mentioning if you weren't aware that Locksmith probably isn't the easiest library to work because it's "protocol-oriented", which is a really new (and cool) idea that I wanted to share. It's certainly not the be all and end all of keychain libraries. Maybe I should add a note on that to the docs too?)

Thanks so much for taking the time to drop in the detailed test case and issue report!

[skington]

Very quickly: yes, those are the same test failures I'm getting. Will now write a longer answer about the rest.

[skington]

The protocol-oriented bit didn't bother me, actually - if anything, it makes me happy about using a library that's probably over-engineered for me at the moment because I suspect I'll be able to carry on using it for more complicated things in the future without getting fed up at limitations.

But then again, at $WORK we use Moose Roles a lot, which are basically the same thing (although Perl being a dynamic language means you can do funkier things with OO).

[skington]

I mentioned CRUD (Create, Read, Update, Delete), which is a standard idiom these days, and having gone back to the documentation one thing that stands out is that it only mentions three things:

Creating, reading, and deleting each have their own protocols: CreateableSecureStorable, ReadableSecureStorable, and DeleteableSecureStorable. And the best part?

You can conform to all three protocols on the same type!

// CreateableSecureStorable lets us create the account in the keychain
try account.createInSecureStore()

// ReadableSecureStorable lets us read the account from the keychain
let result = account.readFromSecureStore()

// DeleteableSecureStorable lets us delete the account from the keychain
try account.deleteFromSecureStore()

I think the documentation should mention how to update an existing record, even if by saying that it's a Keychain-ism that you can't update a record in place, you have to delete it and create it anew instead.

Also, as I think I mentioned in another issue, it's really confusing that the example code talks about creating an account in the keychain, then reading it from the object that we just created. If nothing else, in the sample code above it should use a different variable name from account, e.g.

let accountAgain = TwitterAccount()
let result = accountAgain.readFromSecureStore()

[skington]

Finally, I think some sort of explicit method would be useful, if nothing else because I suspect that there could be annoying race conditions involving threading if something happens between the delete and the create. This is the sort of really annoying intermittent problem that, if it ever happens, is best dealt with at the library level, rather than everyone having to find the problem for themselves. Also, because people used to the CRUD metaphor expect an explicit update method. I'm happy to roll my own for now, though.

[Sega-Zero]

There is a SecItemUpdate which is working fine. I had the same issue in SSKeychain
I fixed an issue with unabling to set a new value with this flow:

//try to set a value
if (![SSKeychain setPassword:pwd forService:svc account:@"test" error:&error] || error)
{
    [SSKeychain updatePassword:@"" forService:svc account:@"test"error:&error];
    [SSKeychain updatePassword:pwd forService:svc  account:@"test"error:&error];
}

I made a patch to SSKeychain.m:

+ (BOOL)updatePassword:(NSString *)password forService:(NSString *)serviceName account:(NSString *)account error:(NSError **)error
{
    NSData *data = [password dataUsingEncoding:NSUTF8StringEncoding];
    return [self updatePasswordData:data forService:serviceName account:account error:error];
}

+ (BOOL)updatePasswordData:(NSData *)password forService:(NSString *)serviceName account:(NSString *)account error:(NSError **)error
{
    OSStatus status = SSKeychainErrorBadArguments;
    if (password && serviceName && account) {
        NSMutableDictionary *query = [self _queryForService:serviceName account:account];
        [query setObject:password forKey:(__bridge id)kSecValueData];
#if __IPHONE_4_0 && TARGET_OS_IPHONE
        NSDictionary *update = @{(__bridge id)kSecValueData:password};
        if (SSKeychainAccessibilityType) {
            [query setObject:(id)[self accessibilityType] forKey:(__bridge id)kSecAttrAccessible];
            update = @{(__bridge id)kSecValueData:password,
                       (__bridge id)kSecAttrAccessible:(id)[self accessibilityType]};
        }
#else
        NSDictionary *update = @{(__bridge id)kSecValueData:password};
#endif

        status = SecItemUpdate((__bridge CFDictionaryRef)query, (__bridge CFDictionaryRef)update);
    }
    if (status != noErr && error != NULL) {
        *error = [NSError errorWithDomain:kSSKeychainErrorDomain code:status userInfo:nil];
    }
    return (status == noErr);
}

You, probably, could use the same technique. Couldn't post a PR to SSKeychain with this fix, but it works for me in my objective-с projects, now I would like to Locksmith to have this behavior too =)
If you want, I could post a PR with swift code.

[matthewpalmer]

Closing because I think this is fixed by @Sega-Zero in #106 :D