CRD iOS: Updating remoting service to use oauth and host list fetcher.

remoting/client/ios/facade/remoting_service.mm

Index: remoting/client/ios/facade/remoting_service.mm
diff --git a/remoting/client/ios/facade/remoting_service.mm b/remoting/client/ios/facade/remoting_service.mm
index 31ac799c5973958c3d0df96f006702c361f3790a..8ed2e0ed6fd589bc8afb9002de2356af782b3a87 100644
--- a/remoting/client/ios/facade/remoting_service.mm
+++ b/remoting/client/ios/facade/remoting_service.mm
@@ -8,13 +8,63 @@
 
 #import <Foundation/Foundation.h>
 
+#import "base/mac/bind_objc_block.h"
 #import "remoting/client/ios/facade/remoting_service.h"

[joedow, 2017/04/05 22:30:56]

In the header the includes were listed before the imports.  Does the order
matter (by convention or functionally)?

[nicholss, 2017/04/07 18:16:15]

Objc guide says this:

The standard order for header inclusion is the header for the current file,
operating system headers, language library headers, and finally groups of
headers for other dependencies.

So I think I have been doing it close but not what the spec says. import first,
then include for each type of import.

I think I still need to understand exactly what the style guide is saying...

 
 #include "base/logging.h"
+#include "base/strings/sys_string_conversions.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "remoting/base/oauth_token_getter.h"
+#include "remoting/base/oauth_token_getter_impl.h"
+#include "remoting/client/ios/facade/host_info.h"
+#include "remoting/client/ios/facade/host_list_fetcher.h"
+
+const char kOauthRedirectUrl[] =
+    "https://chromoting-oauth.talkgadget."
+    "google.com/talkgadget/oauth/chrome-remote-desktop/dev";

[joedow, 2017/04/05 22:30:56]

Typically file scoped vars are put into an anonymous namespace (C++), I'm not
sure if ObjC is different or not but wanted to call it out. 

[nicholss, 2017/04/07 18:16:16]

I have not seen any Obj-C files use namespace yet. A const char in objc is
namespaced to the current file so it kinda does the same thing as an anon name
space already.

+
+std::unique_ptr<remoting::OAuthTokenGetter>
+CreateOAuthTokenGetterWithAuthorizationCode(
+    std::string auth_code,

[joedow, 2017/04/05 22:30:55]

pass string by const ref, unless ObjC is different

[nicholss, 2017/04/07 18:16:16]

This is Obj-C++ so const rules still work the same. Fixed.

+    const remoting::OAuthTokenGetter::CredentialsUpdatedCallback&
+        on_credentials_update) {
+  std::unique_ptr<remoting::OAuthTokenGetter::OAuthIntermediateCredentials>
+      oauth_credentials(
+          new remoting::OAuthTokenGetter::OAuthIntermediateCredentials(
+              auth_code, false));

[joedow, 2017/04/05 22:30:55]

nit: it can be useful to add a comment to bare true/false/int values, i.e.
/*is_service_account=*/false

[nicholss, 2017/04/07 18:16:15]

Done.

+  oauth_credentials->oauth_redirect_uri = kOauthRedirectUrl;
+
+  std::unique_ptr<remoting::OAuthTokenGetter> oauth_tokenGetter(
+      new remoting::OAuthTokenGetterImpl(
+          std::move(oauth_credentials), on_credentials_update,
+          [[RemotingService SharedInstance] runtime]->url_requester(), true));
+  return oauth_tokenGetter;
+}
 
-@interface RemotingService ()
+std::unique_ptr<remoting::OAuthTokenGetter> CreateOAuthTokenWithRefreshToken(
+    std::string refresh_token,

[joedow, 2017/04/05 22:30:55]

pass by const ref for these?

[nicholss, 2017/04/07 18:16:16]

Done.

+    std::string email) {
+  std::unique_ptr<remoting::OAuthTokenGetter::OAuthAuthorizationCredentials>
+      oauth_credentials(
+          new remoting::OAuthTokenGetter::OAuthAuthorizationCredentials(
+              email, refresh_token, false));
+
+  std::unique_ptr<remoting::OAuthTokenGetter> oauth_tokenGetter(
+      new remoting::OAuthTokenGetterImpl(
+          std::move(oauth_credentials),
+          [[RemotingService SharedInstance] runtime]->url_requester(), true));
+  return oauth_tokenGetter;
+}
 
-@property(nonatomic, copy) NSString* authorizationCode;
+@interface RemotingService () {
+  remoting::ChromotingClientRuntime* _runtime;
+  std::unique_ptr<remoting::OAuthTokenGetter> _tokenGetter;
+  UserInfo* _user;
+  NSArray<HostInfo*>* _hosts;
+  id<RemotingAuthenticationDelegate> _authDelegate;
+  id<RemotingHostListDelegate> _hostListDelegate;
+  remoting::HostListFetcher* _hostListFetcher;
+}
 
 @end
 
@@ -23,17 +73,132 @@
 // implemented/integrated yet.
 // TODO(nicholss): Implement/Integrate this class. At the moment it is being
 // used to generate fake data to implement the UI of the app.
+// Update: Half implemented now. User is still fake, but now real hosts lists.
 //
 @implementation RemotingService
 
-@synthesize authorizationCode = _authorizationCode;
+// RemotingService is a singleton.
++ (RemotingService*)SharedInstance {
+  static RemotingService* sharedInstance = nil;
+  static dispatch_once_t guard;
+  dispatch_once(&guard, ^{
+    sharedInstance = [[RemotingService alloc] init];
+  });
+  return sharedInstance;
+}
+
+- (instancetype)init {
+  self = [super init];
+  if (self) {
+    _user = nil;
+    _hosts = nil;
+    _runtime = remoting::ChromotingClientRuntime::GetInstance();
+    _hostListFetcher = new remoting::HostListFetcher(_runtime->url_requester());
+  }
+  return self;
+}
+
+#pragma mark - RemotingService Implementation
+
+// TODO(nicholss): isAuthenticated needs to just kick off a request to
+// authenticate a user. and more than one controller might want to be a delegate
+// for this info so need to change this to be more of the registration types.
+// The remoting_service might also want to be registered for authentication
+// changes and it can update it's cache as it needs.
+
+- (void)setAuthenticationDelegate:(id<RemotingAuthenticationDelegate>)delegate {
+  _authDelegate = delegate;

[joedow, 2017/04/05 22:30:55]

Are you using nil as a way to unregister here?  I think the typical pattern is
add*Observer and remove*Observer, that would also make it easier to maintain a
list of observers so multiple instances could register.

[nicholss, 2017/04/07 18:16:16]

add and set would imply that there can be more than one delegate. I only want
one delegate, setBlahBlah is standard obj-c property setting method name.
Getters = propName, Setter = setPropName

+  if (_authDelegate) {
+    [_authDelegate nowAuthenticated:[self isAuthenticated]];
+  }
+  if (!_user && _tokenGetter) {
+    _tokenGetter->CallWithToken(base::BindBlockArc(
+        ^(remoting::OAuthTokenGetter::Status status,
+          const std::string& user_email, const std::string& access_token) {
+          if (status == remoting::OAuthTokenGetter::Status::SUCCESS) {
+            _user = [[UserInfo alloc] init];
+            _user.userEmail =
+                [NSString stringWithCString:user_email.c_str()
+                                   encoding:[NSString defaultCStringEncoding]];
+          } else {
+            _user = nil;
+          }
+          if (_authDelegate) {
+            [_authDelegate nowAuthenticated:[self isAuthenticated]];
+          }
+        }));
+  }
+}
 
 - (BOOL)isAuthenticated {
-  return self.authorizationCode != nil;
+  if (_user) {
+    return YES;
+  }
+  return NO;
+}
+
+- (void)startHostListFetchWith:(NSString*)accessToken {
+  NSLog(@"startHostListFetchWith : %@ %@", accessToken, _authDelegate);
+  if (_authDelegate) {
+    [_authDelegate nowAuthenticated:YES];
+
+    _hostListFetcher->RetrieveHostlist(
+        base::SysNSStringToUTF8(accessToken),
+        base::BindBlockArc(^(const std::vector<remoting::HostInfo>& hostlist) {
+          NSMutableArray<HostInfo*>* hosts =
+              [NSMutableArray arrayWithCapacity:hostlist.size()];
+          std::string status;
+          for (const remoting::HostInfo& host_info : hostlist) {
+            remoting::HostStatus host_status = host_info.status;
+            if (host_status == remoting::kHostStatusOnline) {

[joedow, 2017/04/05 22:30:55]

nit: you could make this a switch with a default so any changes to the original
enum would cause a compiler error (to tell the engineer to fix this logic as
well).  Up to you for this one though as I'm not sure what new states we would
add.

[nicholss, 2017/04/07 18:16:16]

I like it. Nice idea.

+              status = "ONLINE";
+            } else if (host_status == remoting::kHostStatusOffline) {
+              status = "OFFLINE";
+            } else {
+              status = "UNKNOWN";

[joedow, 2017/04/05 22:30:56]

Log UNREACHED() here?  

[nicholss, 2017/04/07 18:16:16]

was not able to find usage of UNREACHED() , can you explain or show an example?

[joedow, 2017/04/10 16:09:59]

Sorry, the macro is 'NOTREACHED()'.  

[nicholss, 2017/04/10 17:49:53]

Thanks! Updated.

+            }
+
+            // TODO(nicholss): Not yet integrated: createdTime, hostVersion,
+            // kind, offlineReason. Add them as the app will need this info.
+            HostInfo* host = [[HostInfo alloc] init];
+            host.hostId =
+                [NSString stringWithCString:host_info.host_id.c_str()
+                                   encoding:[NSString defaultCStringEncoding]];
+            host.hostName =
+                [NSString stringWithCString:host_info.host_name.c_str()
+                                   encoding:[NSString defaultCStringEncoding]];
+            host.jabberId =
+                [NSString stringWithCString:host_info.host_jid.c_str()
+                                   encoding:[NSString defaultCStringEncoding]];
+            host.publicKey =
+                [NSString stringWithCString:host_info.public_key.c_str()
+                                   encoding:[NSString defaultCStringEncoding]];
+            host.status =
+                [NSString stringWithCString:status.c_str()
+                                   encoding:[NSString defaultCStringEncoding]];
+            [hosts addObject:host];
+          }
+          _hosts = hosts;
+          [_hostListDelegate hostListUpdated];
+        }));
+  }
 }
 
 - (void)authenticateWithAuthorizationCode:(NSString*)authorizationCode {
-  self.authorizationCode = authorizationCode;
+  _tokenGetter = CreateOAuthTokenGetterWithAuthorizationCode(
+      std::string(base::SysNSStringToUTF8(authorizationCode)),
+      base::BindBlockArc(
+          ^(const std::string& user_email, const std::string& refresh_token) {
+            // TODO(nicholss): Do something with these new creds.
+            VLOG(1) << "New Creds: " << user_email << " " << refresh_token;
+          }));
+}
+
+- (void)authenticateWithRefreshToken:(NSString*)refreshToken
+                                emai:(NSString*)email {
+  _tokenGetter = CreateOAuthTokenWithRefreshToken(
+      std::string(base::SysNSStringToUTF8(refreshToken)),
+      base::SysNSStringToUTF8(email));
 }
 
 - (UserInfo*)getUser {
@@ -55,53 +220,30 @@
   return user;
 }
 
+- (void)setHostListDelegate:(id<RemotingHostListDelegate>)delegate {
+  bool attemptUpdate = (_hostListDelegate != delegate);
+  _hostListDelegate = delegate;
+  if (attemptUpdate && _hostListDelegate && _tokenGetter) {
+    _tokenGetter->CallWithToken(base::BindBlockArc(
+        ^(remoting::OAuthTokenGetter::Status status,
+          const std::string& user_email, const std::string& access_token) {
+          NSString* accessToken =
+              [NSString stringWithCString:access_token.c_str()
+                                 encoding:[NSString defaultCStringEncoding]];
+          [self startHostListFetchWith:accessToken];
+        }));
+  }
+}
+
 - (NSArray<HostInfo*>*)getHosts {
   if (![self isAuthenticated]) {
     return nil;
   }
-
-  NSMutableString* json = [[NSMutableString alloc] init];
-  [json
-      appendString:@"{\"data\":{\"kind\":\"chromoting#hostList\",\"items\":["];
-  [json appendString:@"{"];
-  [json appendString:@"\"createdTime\":\"2000-01-01T00:00:01.000Z\","];
-  [json appendString:@"\"hostId\":\"Host1\","];
-  [json appendString:@"\"hostName\":\"HostName1\","];
-  [json appendString:@"\"hostVersion\":\"2.22.5.4\","];
-  [json appendString:@"\"kind\":\"Chromoting#host\","];
-  [json appendString:@"\"jabberId\":\"JabberingOn\","];
-  [json appendString:@"\"publicKey\":\"AAAAABBBBBZZZZZ\","];
-  [json appendString:@"\"status\":\"TESTING\","];
-  [json appendString:@"\"updatedTime\":\"2000-01-01T00:00:01.000Z\""];
-  [json appendString:@"},"];
-  [json appendString:@"{"];
-  [json appendString:@"\"createdTime\":\"2000-01-01T00:00:01.000Z\","];
-  [json appendString:@"\"hostId\":\"Host2\","];
-  [json appendString:@"\"hostName\":\"HostName2\","];
-  [json appendString:@"\"hostVersion\":\"2.22.5.4\","];
-  [json appendString:@"\"kind\":\"Chromoting#host\","];
-  [json appendString:@"\"jabberId\":\"JabberingOn\","];
-  [json appendString:@"\"publicKey\":\"AAAAABBBBBZZZZZ\","];
-  [json appendString:@"\"status\":\"ONLINE\","];
-  [json appendString:@"\"updatedTime\":\"2000-01-01T00:00:01.000Z\""];
-  [json appendString:@"}"];
-  [json appendString:@"]}}"];
-
-  NSMutableData* data = [NSMutableData
-      dataWithData:[[json copy] dataUsingEncoding:NSUTF8StringEncoding]];
-
-  NSMutableArray<HostInfo*>* hosts = [HostInfo parseListFromJSON:data];
-  return hosts;
+  return _hosts;
 }
 
-// RemotingService is a singleton.
-+ (RemotingService*)sharedInstance {
-  static RemotingService* sharedInstance = nil;
-  static dispatch_once_t guard;
-  dispatch_once(&guard, ^{
-    sharedInstance = [[RemotingService alloc] init];
-  });
-  return sharedInstance;
+- (remoting::ChromotingClientRuntime*)runtime {
+  return _runtime;

[joedow, 2017/04/05 22:30:56]

Can you just return 'remoting::ChromotingClientRuntime::GetInstance()' here and
remove the member variable?

[nicholss, 2017/04/07 18:16:15]

Done.

 }
 
 @end