Custom Key Prefix

[bravedrake]

I've made some changes to allow for a custom key prefix to be specified, defaulting to the bundle id.

The reason for this is that I am working with iOS app extensions, which have different bundle id's from the main app, but which need access to the keychain of the main app for login credentials.

The implementation is possibly a bit controversial, as I have changed Lockbox to be instantiable. The first commit simply added a class method, but this seemed unsafe, as setting this after calling some of the setters would mean that the prefix would change, and the previously set key/value pair would be unaccessible. Making the class instantiable means that the prefix has to be set up front, and cannot be changed.

[granoff]

Thanks for this. You've clearly invested some time and thought into this. I like the idea. I am not sure I like this implementation that essentially replaces the class method interfaces with something equivalent but totally different.

I can envision an implementation of this that maintains the class methods, that access a file static singleton instance of Lockbox, taking advantage under the hood of what you have done here.

So, for existing code bases that upgrade to a new version of Lockbox, and have no need for a different key prefix, they can continue to use the code without worry. Under the hood, an internal Lockbox instance using the bundle id as the key prefix would get used. But for needs such as you have outlined, anyone could instantiate their own instance of Lockbox and use the instance methods directly.

Does that make sense? I love what you've done, but I don't want to lose the simpler class method-based APIs.

[bravedrake]

Sure, it makes complete sense not to force everyone to change their implementation.

My only concern really is that the Lockbox class is going to be a little confusing, as there will be essentially duplicate versions of every getter and setter (one for class and another for instance).

Only solution to this that I can think of is to move the code I have modified here in to a separate class that could be instantiated for use cases like mine, and then have the Lockbox class do as you suggest, i.e. have class methods and internally use an instance of the new class? Not sure what I would name that new class though.

I'm happy to give whatever you think is best a go though, so just let me know.

[granoff]

No secondary class is needed.

Add a static instance of Lockbox to Lockbox.m:

static Lockbox *_lockbox = nil;

And add back the class initializer:

+(void)initialize
{
    dispatch_once( ... ) {
        _lockbox = [Lockbox new];
    }
}

As for your class methods, they just call their counterpart methods on the static instance, for example:

+(void)setString ...
{
    [_lockbox setString ...];
}

So, yeah, you end up with class methods that just call the instance methods, but there's no duplication of code, and you'll retain the class method level of convenience without breaking older code bases.

Does that help?

[bravedrake]

I was thinking more about confusion in the header, as you'd have for example both

 +(BOOL)setString:(NSString *)value forKey:(NSString *)key;

...and...

 -(BOOL)setString:(NSString *)value forKey:(NSString *)key;

If you create the static instance in the class initializer, then you will always have that static instance, regardless of whether you use it or not. I guess it's not really much overhead, but just thought I'd mention it.

[bravedrake]

I've hit a bit of a blocker with the unit tests, as I want to test that both the class and instance methods have the already specified results, but I don't really want to copy and paste all of the tests, as this isn;t really maintainable.

To me it seems like trying to keep both the static and class methods will turn what is a nice and simple library in to a bit of a mess. I can't think of a good solution that will maintain the static methods, so can I suggest that a future version moves towards an instantiable model, perhaps with the option of a public singleton instance that would ease the migration for exising users?

[granoff]

Yes, you'd need both class-level and instance method signatures, so in that sense, I see how it could be confusing. Feel free to update the README with a section detailing why someone would use the class methods vs instantiating their own Lockbox instance to get at the custom key prefix functionality. Documentation is the way to resolve the potential for confusion.

[granoff]

I'm happy to take what you've done (the core of it to add the custom key capability) and move towards the model we're talking about that maintains both the class and instance level methods. It's a great enhancement. I'll start with some of what you've done, of course.

[granoff]

That is what your last comment suggested, right?

[granoff]

I ran with the ball... have a look at the branch custom-key-prefix and see what you think. Just one commit there, so should be easy to look at. Give it a try. If it meets your needs, I'll merge it to master and update the Pod.

[bravedrake]

Excellent, thanks for that. I've just looked at that branch, but there are no commits ahead of master on it.

[granoff]

Right. I have not committed this yet to master because I wanted to give you the opportunity to see if what I did reflects the spirit and capability of what you intended with your original pull request. Sounds like it does?

[bravedrake]

Sorry, I didn't make myself clear. I can see that you have created a branch named custom-key-prefix, but I don't think you've pushed your commits, as there are no commits on that branch. See the comparison of master versus custom-key-prefix: https://github.com/granoff/Lockbox/compare/custom-key-prefix

[granoff]

Oh good lord. I'm an idiot. :-) Yesterday was crazy. Try it now. Sorry!

[bravedrake]

Haha, no worries! Just tried it, and works great for me, thanks.

[granoff]

The code I worked up that embodies this idea while retaining backward compatibility has been merged to master. So, closing this pull request. Thanks @bravedrake for this great idea and your original work!

[bravedrake]

Sorry, only just got around to trying this out. I can see the podspec on master has been updated to 2.1.0, but I don't think it has been pushed to cocoapods, as the latest version there is 2.0.0?

[granoff]

Ack! I always forget that part. :-) Pushed now to CocoaPods. Sorry.

[bravedrake]

Excellent, using it now. Thanks.