reidmain / FDKeychain

Save, load and delete items from the keychain with a single Objective-C message.
MIT License
119 stars 21 forks source link

Update documentation + add support for 'kSecAttrAccessibleAlways' #11

Closed Supertecnoboff closed 7 years ago

Supertecnoboff commented 7 years ago

Hi,

As I mentioned in the email I sent you, this is a superb library! However I do think a few improvements need to be made:

1) Add support for 'kSecAttrAccessibleAlways'. Some people believe Apple deprecated 'kSecAttrAccessibleAlways', however thats NOT true (https://developer.apple.com/reference/security/ksecattraccessiblealways). I am well aware that Apple does not recommend using this type, however there are certain use cases that warrant its use, such as: App Widgets and other extensions that can be accessed, even when the device is locked. My app includes a simple widget that displays user account stats, therefore it needs access to the keychain, even when the device is locked as the user can access the widget from the lock screen (as of iOS 10).

2) Documentation update - The (access group) loading code type, needs to be changed to NSString... really not sure what the OAuthToken type is (as its undeclared).

    NSError *error = nil;

    NSString *token = [FDKeychain itemForKey: @"token" 
    forService: @"Moon Unit" 
    inAccessGroup: @"XXXXXXXXXX.com.1414degrees.moonunit" 
    error: &error];

3) Documentation update - The (access group) saving code needs to be updated to show the new save method header (as the current one is not defined anymore):

    NSError *token_error = nil;

    [FDKeychain saveItem: token forKey: @"token" 
    forService: @"Moon Unit" inAccessGroup: @"XXXXXXXXXX.com.1414degrees.moonunit" 
    withAccessibility: FDKeychainAccessibleAlways error:&token_error];

In regards to my first point, I corrected it on my local version of FDKeychain by making the following changes:

In the header file, I updated the FDKeychainAccessibility typedef to:

typedef NS_ENUM(NSInteger, FDKeychainAccessibility) {

    /// The item in the keychain can only be accessed while the device is unlocked.
    FDKeychainAccessibleWhenUnlocked,

    /// The item in the keychain cannot be accessed after a restart until the device has been unlocked once.
    FDKeychainAccessibleAfterFirstUnlock,

    /// The item in the keychain can always be accessed, regardless of the device (un)lock state.
    /// This is NOT recommended by Apple, however certain use cases (ie: App Widget/Extension) warrant its use.
    FDKeychainAccessibleAlways,
};

And in the implementation file, I added the following cases to the switch statements in the saveItem:(id<NSCoding>)item forKey: (NSString *)key forService: (NSString *)service inAccessGroup: (NSString *)accessGroup withAccessibility: (FDKeychainAccessibility)accessibility error: (NSError **)error method:

case FDKeychainAccessibleAlways: {
     [attributes setObject: (__bridge id)kSecAttrAccessibleAlways forKey: (__bridge id)kSecAttrAccessible];
     break;
}

case FDKeychainAccessibleAlways: {
     [attributesToUpdate setObject: (__bridge id)kSecAttrAccessibleAlways forKey: (__bridge id)kSecAttrAccessible];
     break;
}
reidmain commented 7 years ago

1) Support for kSecAttrAccessibleAlways was addressed inhttps://github.com/reidmain/FDKeychain/pull/1#issuecomment-133593138. In my opinion there is never a need to use kSecAttrAccessibleAlways because if a user has not unlocked their device at least once then there is no reason to display any data resulting from using whatever was stored in the keychain. If this is the case then you will get back an NSError with the code errSecInteractionNotAllowed to know that you weren't able to access the keychain and can handle that case appropriately. In the situation of a widget you could display some text to the user indicating that they should unlock their phone at least once.

2) The method itemForKey:forService:inAccessGroup:error: (NSError **)error returns an id which means you can type the result to anything you want. The documentation is indicating a situation where the user stored a OAuthToken in the keychain and is now querying it back. You are able to store any object in the keychain as long as it adheres to the NSCoding protocol.

3) Good catch I have updated that.

Supertecnoboff commented 7 years ago

@reidmain thank you for your response.

1) I have thought about this more and to be honest with you, I now agree with you. The more I think about it, the more I realise that a) the widget shouldn't be doing anything too intensive (i.e.: performing actions on behalf of a logged in user/changing settings/etc...) and b) a single device unlock is not going to cause the user discomfort lol

2) That is fair enough. I just meant that a novice programmer might not understand why a simple copy/paste code snippet, is coming up with an undeclared error.

3) Cheers 👍

Thanks, Dan.

reidmain commented 7 years ago

@Supertecnoboff good point. I didn't think about that. When I wrote that documentation a couple years ago I think the goto OAuth library at that point had a OAuthToken object.

I will update the documentation such that there is a little blurb "Assuming you have a OAuthToken class" and I'll put a little block of code like

@interface OAuthToken : NSObject<NSCoding>
...

to showcase how custom objects can be stored in the keychain.

Supertecnoboff commented 7 years ago

@reidmain Great, thanks very much 👍