MOB-415: Add access to access token and save it in the keychain #11

mats-claassen · 2016-12-12T14:33:50Z

Fixes #6

Adds access token management code according to IDme/ID.me-WebVerify-SDK-Android#9

Also adds SAMKeychain dependency for keychain access. Dependency is added as a pod dependency.

m-revetria · 2016-12-12T16:01:27Z

ID.me WebVerify SDK/IDmeWebVerify.m


 /// API Constants (Production)
 #define IDME_WEB_VERIFY_GET_AUTH_URI                    @"https://api.id.me/oauth/authorize?client_id=%@&redirect_uri=%@&response_type=token&scope=%@"
 #define IDME_WEB_VERIFY_GET_USER_PROFILE                @"https://api.id.me/api/public/v2/%@.json?access_token=%@"

 /// Data Constants
 #define IDME_WEB_VERIFY_ACCESS_TOKEN_PARAM              @"access_token"
+#define IDME_WEB_VERIFY_EXPIRATION_PARAM              @"expires_in"


please keep expires_in at the same column of access_token

m-revetria · 2016-12-12T16:04:22Z

ID.me WebVerify SDK/IDmeWebVerify.m

        [self clearWebViewCacheAndCookies];
    }

    return self;
 }

+ (void)initializeWithClientID:(NSString *)clientID redirectURI:(NSString *)redirectURI {
+    [[IDmeWebVerify sharedInstance] setClientID:clientID];


do we want to support changing the clientID after the initialize method was called? I think these properties should be readonly and here we should call a private constructor which pass the id and url

Currently sharedInstance is initializing the instance. Should we require calling initialize... before calling sharedInstance and then calling the init from the initialize?

m-revetria · 2016-12-12T16:32:29Z

ID.me WebVerify SDK/IDmeWebVerify.h

+ @param forceRefreshing Force the SDK to refresh the token and do not use the current one.
+ @param callback A block that returns an NSString object representing a valid access token or an NSError object.
+ */
+- (void)getAccessTokenWithScope:(NSString*)scope forceRefreshing:(BOOL)force result:(IDmeVerifyWebVerifyResults _Nonnull)callback;


please add nullability indicator to scope

m-revetria · 2016-12-12T16:34:00Z

ID.me WebVerify SDK/IDmeWebVerify.m

+                NSHTTPURLResponse* httpResponse = (NSHTTPURLResponse*)response;
+                NSUInteger statusCode = [httpResponse statusCode];
+                if ([data length]) {
+                    NSDictionary *results = (NSDictionary *)[NSJSONSerialization JSONObjectWithData:data options:NSJSONReadingAllowFragments error:nil];


pass an error variable and check if everything was ok

m-revetria · 2016-12-12T16:36:37Z

ID.me WebVerify SDK/IDmeWebVerify.m

+    [self.keychainData clean];
+}
+
+- (void)getAccessTokenWithScope:(NSString*)scope forceRefreshing:(BOOL)force result:(IDmeVerifyWebVerifyResults)callback{


nullability indicators are missing

m-revetria · 2016-12-12T16:42:08Z

ID.me WebVerify SDK/IDmeWebVerify.m

@@ -343,4 +384,21 @@ - (NSString * _Nullable)clientID{

    return (_clientID) ? _clientID : nil;
 }
+
+#pragma mark - Helpers
+- (void)setExpirationDateWith:(NSNumber*)seconds{


please add nullability indicator

m-revetria · 2016-12-12T16:43:32Z

ID.me WebVerify SDK/IDmeWebVerify.m

+    self.keychainData.expirationDate = [NSDate dateWithTimeIntervalSinceNow:[seconds doubleValue]];
+}
+
+- (UIViewController*) topMostController


please add nullability indicator

Additionally, I'm not sure if we should use this here, it is related to the getAccessToken feature

m-revetria · 2016-12-12T16:55:40Z

ID.me WebVerify SDK/IDmeWebVerify.m

+        return;
+    } else if (![scope isEqualToString:self.keychainData.scope]) {
+        [self logout];
+        [self verifyUserInViewController:[self topMostController] scope:scope withTokenResult:callback];


I don't think we should show the login form automatically. This function getAccessToken is intended to just get a fresh token to make an API call, for example. Remember that this could be done in a background thread where the developer maybe don't want or can't show some UI. In this case I would call the callback with an error. This mostly must be defined in the issue discussion

This depends on how scope change is handled. I would think that scope change would require a new login and should therefore not be called from a background thread. But yes it should be discussed in the issue.

m-revetria · 2016-12-12T17:00:55Z

ID.me WebVerify SDK/IDmeWebVerifyKeychainData.m

+
+    NSError* error;
+    [SAMKeychain setPasswordData:dictionaryRep forService:[NSBundle mainBundle].bundleIdentifier account:IDME_KEYCHAIN_DATA_ACCOUNT error:&error];
+    NSLog(@"%@", error);


if persist fails, shouldn't fail the entire sign in process?

Not sure. Keychain might fail but the sign in succeeded. So the user will be able to use the app while he does not shut it down. He would have to sign in again after shutting down the app as the token has not been saved to the keychain. But it is better than make the sign in fail when it succeeded in reality.

m-revetria · 2016-12-12T17:01:35Z

ID.me WebVerify SDK/IDmeWebVerifyKeychainData.m

+-(void)loadFromKeychain{
+    NSString *error;
+    NSData* data = [SAMKeychain passwordDataForService:[NSBundle mainBundle].bundleIdentifier account:IDME_KEYCHAIN_DATA_ACCOUNT];
+    NSDictionary *dictionary = [NSPropertyListSerialization propertyListFromData:data mutabilityOption:NSPropertyListImmutable format:nil errorDescription:&error];


we should send back this error

This method is internal and just called in the init

mirland · 2016-12-12T18:13:45Z

ID.me WebVerify SDK/IDmeWebVerify.h

- @param clientID The clientID provided by ID.me when registering the app at @b http://developer.id.me
- @param redierectURI The redirectURI provided to ID.me when registering your app at @b http://developer.id.me
- @param affiliationType The type of group verficiation that should be presented. Check the @c IDmeVerifyAffiliationType typedef for more details
+ @param scope The type of group verficiation that should be presented.


typo in verification

Copied it from the previous method 😱

mats-claassen · 2016-12-12T19:14:00Z

PR yet depending on scope handling discussion

sdownie · 2016-12-13T16:52:20Z

ID.me WebVerify SDK/IDmeWebVerify.m

+        NSDate* expiration = self.keychainData.expirationDate;
+        NSDate* now = [[NSDate alloc] init];
+        if ([now compare:expiration] != NSOrderedAscending) {
+            // TODO: refresh token


@mats-claassen Are you planning on adding refreshing tokens in this branch?

No. I want to implement that in a separate branch linking to #7

Addresses #6

m-revetria · 2016-12-13T18:54:52Z

ID.me WebVerify SDK/IDmeWebVerify.h

 @param webVerificationResults A block that returns an NSDictionary object and an NSError object. The verified user's profile is stored in an @c NSDictionary object as @c JSON data. If no data was returned, or an error occured, @c NSDictionary is @c nil and @c NSError returns an error code and localized description of the specific error that occured.
 */
- (void)getUserProfileWithResult:(IDmeVerifyWebVerifyResults _Nonnull)webVerificationResults;
+- (void)getUserProfileWithScope:(NSString* _Nullable)scope andResult:(IDmeVerifyWebVerifyResults _Nonnull)webVerificationResults;


Don’t use “and” to link keywords that are attributes of the receiver. (https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingMethods.html#//apple_ref/doc/uid/20001282-BCIGIJJF)

m-revetria · 2016-12-13T19:00:21Z

ID.me WebVerify SDK/IDmeWebVerify.m

+                                        NSUInteger statusCode = [httpResponse statusCode];
+                                        if ([data length]) {
+                                            NSError* error;
+                                            NSDictionary *results = (NSDictionary *)[NSJSONSerialization JSONObjectWithData:data options:NSJSONReadingAllowFragments error:&error];


this is only needed if statusCode == 200, right? maybe you can move it to that if.

I'm not sure about the structure of this if else block. What would happen if statusCode == 401 and the response doesn't have a body? Currently it is returning the error IDmeWebVerifyErrorCodeVerificationDidFailToFetchUserProfile instead of doing the refresh process.

What do you think?

m-revetria · 2016-12-13T19:02:03Z

ID.me WebVerify SDK/IDmeWebVerify.m

+
+        if (!latestScope) {
+            // no token has been requested yet
+            callback(nil, nil, nil);


shouldn't an error be returned in this case?

This happens when there has never been a login. We could return an error IDmeWebVerifyErrorCodeNoSuchScope

m-revetria · 2016-12-13T19:04:27Z

ID.me WebVerify SDK/IDmeWebVerifyKeychainData.h

+-(NSDate* _Nullable)expirationDateForScope:(NSString* _Nonnull)scope;
+-(NSString* _Nullable)refreshTokenForScope:(NSString* _Nonnull)scope;
+
+-(void)setToken:(NSString * _Nonnull)accessToken expirationDate:(NSDate * _Nonnull)date


please move each parameter to a new line to improve readability

m-revetria · 2016-12-13T19:07:53Z

ID.me WebVerify SDK/IDmeWebVerifyKeychainData.m

+                                                                             options:NSPropertyListMutableContainersAndLeaves
+                                                                              format:nil
+                                                                               error:&error];
+        self.tokensByScope = [NSMutableDictionary dictionaryWithDictionary:dictionary];


shouldn't you set latestScope at this moment?

m-revetria · 2016-12-13T19:09:23Z

ID.me WebVerify SDK/IDmeWebVerifyKeychainData.h

+-(NSString* _Nullable)refreshTokenForScope:(NSString* _Nonnull)scope;
+
+-(void)setToken:(NSString * _Nonnull)accessToken expirationDate:(NSDate * _Nonnull)date
+   refreshToken:(NSString * _Nullable)refreshToken forScope:(NSString * _Nonnull)scope;


refresh token must not be nullable, right? maybe add a todo to change it when we are implementing refresh feature

It is nullable as it will not be updated when the access token gets updated so passing nil will leave the old refresh token. We can change this if it is not clear. Or we should document it

m-revetria · 2016-12-13T19:12:04Z

ID.me WebVerify SDK/IDmeWebVerify.h

 @param webVerificationResults A block that returns an NSDictionary object and an NSError object. The verified user's profile is stored in an @c NSDictionary object as @c JSON data. If no data was returned, or an error occured, @c NSDictionary is @c nil and @c NSError returns an error code and localized description of the specific error that occured.
 */
- (void)getUserProfileWithResult:(IDmeVerifyWebVerifyResults _Nonnull)webVerificationResults;
+- (void)getUserProfileWithScope:(NSString* _Nullable)scope andResult:(IDmeVerifyWebVerifyResults _Nonnull)webVerificationResults;


I wouldn't use the same block type for getting the token and user profile due to them should have different argument list. When requesting the access token doesn't matter the profile parameter, and when requesting the user profile probably you don't want the access token.

What do you think? Maybe this is a change for a new PR

I agree. I did just keep the block as it was in the previous version.

minor fixes

m-revetria · 2016-12-15T18:39:59Z

ID.me WebVerify SDK/IDmeWebVerify.m


    if (force) {
        // TODO: refresh token
-        callback(nil, nil, nil);
+        callback(nil, nil);


due to we won't support refresh token feature for a time, what do you think about sending back a not-implemented-like error?

Maybe we should remove the force for now.

m-revetria · 2016-12-15T18:40:37Z

ID.me WebVerify SDK/IDmeWebVerify.m

@@ -204,16 +192,13 @@ - (void)getAccessTokenWithScope:(NSString* _Nullable)scope forceRefreshing:(BOOL
        NSDate* now = [[NSDate alloc] init];
        if ([now compare:expiration] != NSOrderedAscending) {
            // TODO: refresh token
-            callback(nil, nil, nil);
+            callback(nil, nil);


... and here return an unauthorized error?

m-revetria · 2016-12-15T18:45:50Z

Sample Project/WebVerifySample/ViewController.m

@@ -119,12 +119,12 @@ - (void)verifyAction:(id)sender {
 }

 - (void)tokenTestAction:(id)sender {
-    [[IDmeWebVerify sharedInstance] getUserProfileWithResult:^(NSDictionary *userProfile, NSError *error, NSString *accessToken) {
+    [[IDmeWebVerify sharedInstance] getUserProfileWithScope: @"wallet" result:^(NSDictionary *userProfile, NSError *error) {


shouldn't we use some test scope? do we want wallet to be public?

Sorry this should not be there

sdownie · 2016-12-19T14:54:52Z

ID.me WebVerify SDK/IDmeWebVerifyKeychainData.m

+/// Keychain Constants
+#define IDME_KEYCHAIN_DATA_ACCOUNT              @"IDME_KEYCHAIN_DATA"
+#define IDME_EXPIRATION_DATE                    @"IDME_EXPIRATION_DATE"
+#define IDME_REFRESH_TOKEN                      @"IDME_REFRESH_TOKEN"


@mats-claassen We should drop refresh token for now

Everything is implemented so that a refresh token can be easily supported in the future. Do you want us to remove everything related to refresh tokens?

sdownie · 2016-12-19T14:57:34Z

ID.me WebVerify SDK/IDmeWebVerifyKeychainData.m

+//  IDmeWebVerifyKeychainData.m
+//  WebVerifySample
+//
+//  Created by Mathias Claassen on 12/9/16.


@mats-claassen We should be able to drop the "created by" since we're using git :)

MOB-415: Add access to access token and save it in the keychain Fixes #6 * Handle several scopes and their associated access tokens * Separate the callback block for profile and token results. * Throw appropriate errors when there is no valid token

mats-claassen requested review from sdownie and m-revetria December 12, 2016 14:33

m-revetria suggested changes Dec 12, 2016

View reviewed changes

mirland reviewed Dec 12, 2016

View reviewed changes

sdownie reviewed Dec 13, 2016

View reviewed changes

mats-claassen added 2 commits December 13, 2016 15:15

MOB-415: Add access to access token and save it in the keychain

0a74af7

Addresses #6

fix typos, missing nullability and other errors

af920db

mats-claassen force-pushed the MOB-415 branch from 404ef04 to f10b818 Compare December 13, 2016 18:17

handle several scopes and their associated access tokens

07d2100

mats-claassen force-pushed the MOB-415 branch from f10b818 to 07d2100 Compare December 13, 2016 18:22

m-revetria suggested changes Dec 13, 2016

View reviewed changes

separate the callback block for profile and token results.

8f2d2d2

minor fixes

m-revetria reviewed Dec 15, 2016

View reviewed changes

throw appropriate errors when there is no valid token

8f531ec

m-revetria approved these changes Dec 16, 2016

View reviewed changes

sdownie reviewed Dec 19, 2016

View reviewed changes

fix author notes

4bd73ee

sdownie approved these changes Dec 19, 2016

View reviewed changes

mats-claassen merged commit 063a7c9 into 4.x Dec 19, 2016

mats-claassen deleted the MOB-415 branch December 19, 2016 18:17

mats-claassen mentioned this pull request Mar 28, 2017

Release 4.0.0 #34

Merged

MOB-415: Add access to access token and save it in the keychain #11

MOB-415: Add access to access token and save it in the keychain #11

Conversation

mats-claassen commented Dec 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mats-claassen commented Dec 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment