[CRD iOS] Implementing save to keychain and user defaults for login info. Refactor remoting service.

In this CL I add a keychain wrapper and save the refresh token for a current user. 
This will load the stored data on app launch, which will let us work more quickly 
in Chromium without needing to login each app launch.

This also refactors the remoting service to move all authentication related code into
a class that manages authentication. 

Moving to a better pattern of using @properties vs methods.

Adding eventing on hosts update and user update from the service.

Review-Url: https://codereview.chromium.org/2854273002
Cr-Commit-Position: refs/heads/master@{#470363}
Committed: https://chromium.googlesource.com/chromium/src/+/c372b3356c8ff8049754aa5bb2be014d32353ed8

[nicholss, 2017-05-03 21:01:19]

Description was changed from

==========
Implementing save to keystore for Chromium.
==========

to

==========
In this CL I add a keychain wrapper and save the refresh token for a current
user. This will load the stored data on app launch, which will let us work more
quickly in Chromium without needing to login each app launch.
==========

[nicholss, 2017-05-03 21:03:49]

Description was changed from

==========
In this CL I add a keychain wrapper and save the refresh token for a current
user. This will load the stored data on app launch, which will let us work more
quickly in Chromium without needing to login each app launch.
==========

to

==========
In this CL I add a keychain wrapper and save the refresh token for a current
user. 
This will load the stored data on app launch, which will let us work more
quickly 
in Chromium without needing to login each app launch.

This also refactors the remoting service to move all authentication related code
into
a class that manages authentication. 

Moving to a better pattern of using @properties vs methods.

Adding eventing on hosts update and user update from the service.
==========

[nicholss, 2017-05-03 21:03:49]

nicholss@chromium.org changed reviewers:
+ yuweih@chromium.org

[nicholss, 2017-05-03 21:03:59]

PTAL thanks!

[Yuwei, 2017-05-04 05:10:15]

It could be much easier to review if you had broken it down into multiple
smaller CLs O_o

Also have you looked at AppleKeychain?

https://cs.chromium.org/chromium/src/crypto/apple_keychain.h

It might be useful

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/do...
File remoting/client/ios/domain/user_info.mm (right):

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/do...
remoting/client/ios/domain/user_info.mm:36: if (_userEmail && _userEmail.length
> 0 && _refreshToken &&
Just return _userEmail && _userEmail.length > 0 && _refreshToken &&
_refreshToken.length > 0?

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
File remoting/client/ios/facade/remoting_authentication.h (right):

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_authentication.h:39: -
(void)callbackWithAccessToken:
This method doesn't really fetch the OAuth token if none of authenticateWith*
has been called. Fix this comment and maybe also fix the method name?

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
File remoting/client/ios/facade/remoting_authentication.mm (right):

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_authentication.mm:30: const char
kOauthRedirectUrl[] =
Put const and functions below inside an empty namespace?

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_authentication.mm:48: [[RemotingService
SharedInstance] runtime]->url_requester(),
Now that runtime is a property, use .runtime instead?

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_authentication.mm:102: -
(void)setUser:(UserInfo*)user {
Is this only privately used? If so maybe mark |user| readonly and provide a
private updateUser method?

Technically speaking if the caller calls setUsers then sounds like that UserInfo
should be stored in defaults/keychain..

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_authentication.mm:104: [_delegate
userDidUpdate:_user];
This will be called with nil _user if loadUserInfo returns nil. Is this
expected?

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_authentication.mm:119: [self
storeUserInfo:_user];
Use weakSelf? |credentials_updated_callback_| in the token getter never gets
reset.

See:
https://cs.chromium.org/chromium/src/ios/web/net/crw_ssl_status_updater.mm?l=...

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_authentication.mm:140: _tokenGetter =
CreateOAuthTokenWithRefreshToken(
Update _user and store here?

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_authentication.mm:148:
_tokenGetter->CallWithToken(onAccessToken);
Be careful here since a failure to reset onAccessToken will end up with retain
cycle and memory leakage.

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
File remoting/client/ios/facade/remoting_service.h (right):

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_service.h:35: // Logout of the current
account and clear data.
It's kind of confusing that this class provides methods similar to those in
RemotingAuthentication but they don't work exactly the same. One could be
tempted to call logout in |authentication| and found that the host list doesn't
get cleared.

I would prefer either completely encapulating |authentication| or handle the
user == nil case in the userDidUpdate callback (if it still gets called in user
== nil case, otherwise you may add a userDidLogOut).

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_service.h:45: @property(nonatomic, readonly)
UserInfo* user;
I'm not sure how useful this can be given that it's just a shortcut to
authentication.user. One may hesitate to choose user vs authentication.user
without looking at the implementation.

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
File remoting/client/ios/facade/remoting_service.mm (right):

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_service.mm:92: status = "ONLINE";
Not related to this CL. Why is the status a string rather than an enum?

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_service.mm:121: [self hostListUpdated];
_hostListFetcher doesn't seem to reset the callback in case of failure, which
may form retain cycle. Pass a weakSelf here?

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/ke...
File remoting/client/ios/keychain_wrapper.h (right):

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/ke...
remoting/client/ios/keychain_wrapper.h:18: -
(void)setRefreshToken:(NSString*)refreshToken;
What about using @property?

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/ke...
File remoting/client/ios/keychain_wrapper.mm (right):

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/ke...
remoting/client/ios/keychain_wrapper.mm:13: static const UInt8
kKeychainItemIdentifier[] = "org.chromium.RemoteDesktop\0";
Why is there an extra zero-terminator? I think the string literal will
automatically add it?

[nicholss, 2017-05-08 17:08:10]

PTAL

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/do...
File remoting/client/ios/domain/user_info.mm (right):

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/do...
remoting/client/ios/domain/user_info.mm:36: if (_userEmail && _userEmail.length
> 0 && _refreshToken &&
On 2017/05/04 05:10:13, Yuwei wrote:
> Just return _userEmail && _userEmail.length > 0 && _refreshToken &&
> _refreshToken.length > 0?

I tend avoid return the result of a wacky logic test because it makes it hard to
understand the result.  This is for readability.

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
File remoting/client/ios/facade/remoting_authentication.h (right):

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_authentication.h:39: -
(void)callbackWithAccessToken:
On 2017/05/04 05:10:14, Yuwei wrote:
> This method doesn't really fetch the OAuth token if none of authenticateWith*
> has been called. Fix this comment and maybe also fix the method name?

I think the method name is ok, I added a comment. But in time I think it will
need to be 
refactored to add an error code as well. Added a TODO for that.

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
File remoting/client/ios/facade/remoting_authentication.mm (right):

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_authentication.mm:30: const char
kOauthRedirectUrl[] =
On 2017/05/04 05:10:14, Yuwei wrote:
> Put const and functions below inside an empty namespace?

you get that for free inside a obj-c file.

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_authentication.mm:48: [[RemotingService
SharedInstance] runtime]->url_requester(),
On 2017/05/04 05:10:14, Yuwei wrote:
> Now that runtime is a property, use .runtime instead?

Done.

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_authentication.mm:102: -
(void)setUser:(UserInfo*)user {
On 2017/05/04 05:10:14, Yuwei wrote:
> Is this only privately used? If so maybe mark |user| readonly and provide a
> private updateUser method?
> 
> Technically speaking if the caller calls setUsers then sounds like that
UserInfo
> should be stored in defaults/keychain..

The private _user was cruft. Removed. also switched around calls to
storeUserInfo to only happen from setUser.

Good catch.

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_authentication.mm:104: [_delegate
userDidUpdate:_user];
On 2017/05/04 05:10:14, Yuwei wrote:
> This will be called with nil _user if loadUserInfo returns nil. Is this
> expected?

yes it is.

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_authentication.mm:119: [self
storeUserInfo:_user];
On 2017/05/04 05:10:14, Yuwei wrote:
> Use weakSelf? |credentials_updated_callback_| in the token getter never gets
> reset.
> 
> See:
>
https://cs.chromium.org/chromium/src/ios/web/net/crw_ssl_status_updater.mm?l=...

Done.

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_authentication.mm:140: _tokenGetter =
CreateOAuthTokenWithRefreshToken(
On 2017/05/04 05:10:14, Yuwei wrote:
> Update _user and store here?

The refresh token comes from an active _user object. See the call from
loadUserInfo.

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_authentication.mm:148:
_tokenGetter->CallWithToken(onAccessToken);
On 2017/05/04 05:10:14, Yuwei wrote:
> Be careful here since a failure to reset onAccessToken will end up with retain
> cycle and memory leakage.

Added a TODO.

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
File remoting/client/ios/facade/remoting_service.h (right):

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_service.h:35: // Logout of the current
account and clear data.
On 2017/05/04 05:10:14, Yuwei wrote:
> It's kind of confusing that this class provides methods similar to those in
> RemotingAuthentication but they don't work exactly the same. One could be
> tempted to call logout in |authentication| and found that the host list
doesn't
> get cleared.
> 
> I would prefer either completely encapulating |authentication| or handle the
> user == nil case in the userDidUpdate callback (if it still gets called in
user
> == nil case, otherwise you may add a userDidLogOut).

I like the fact that authentication object is exposed. I moved logout to be only
in authentication. Service is notified if user goes nil and acts on the info.

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_service.h:45: @property(nonatomic, readonly)
UserInfo* user;
On 2017/05/04 05:10:14, Yuwei wrote:
> I'm not sure how useful this can be given that it's just a shortcut to
> authentication.user. One may hesitate to choose user vs authentication.user
> without looking at the implementation.

I was not 100% sure how the api would end up but I settled on the user living in
the auth object and this was not needed. Removed.

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
File remoting/client/ios/facade/remoting_service.mm (right):

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_service.mm:92: status = "ONLINE";
On 2017/05/04 05:10:14, Yuwei wrote:
> Not related to this CL. Why is the status a string rather than an enum?

I was displaying this status in the UI for testing.

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_service.mm:121: [self hostListUpdated];
On 2017/05/04 05:10:14, Yuwei wrote:
> _hostListFetcher doesn't seem to reset the callback in case of failure, which
> may form retain cycle. Pass a weakSelf here?

the callback is cleared with base::ResetAndReturn. This should get called
regardless of error:

https://cs.chromium.org/chromium/src/net/url_request/url_fetcher_delegate.h?t...

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/ke...
File remoting/client/ios/keychain_wrapper.h (right):

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/ke...
remoting/client/ios/keychain_wrapper.h:18: -
(void)setRefreshToken:(NSString*)refreshToken;
On 2017/05/04 05:10:15, Yuwei wrote:
> What about using @property?

In this case I think it is not good to use @property because of the fact this is
a wrapper and I am not 100% happy about this API, and I need it to be
interchangeable with Chrome/Chromium version of this concept. This will have to
be refactored when the integration with SSO happens but I am not 100% sure what
that needs to look like yet so I want to hold off refactoring this until I can
understand the SSO side of things a bit better.

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/ke...
File remoting/client/ios/keychain_wrapper.mm (right):

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/ke...
remoting/client/ios/keychain_wrapper.mm:13: static const UInt8
kKeychainItemIdentifier[] = "org.chromium.RemoteDesktop\0";
On 2017/05/04 05:10:15, Yuwei wrote:
> Why is there an extra zero-terminator? I think the string literal will
> automatically add it?

I yanked this from some apple docs. This is what they were using, I think it is
because it is not a string but an array of chars that is getting assigned into a
UInt8 array. So I do think you need to null term the string here.

[nicholss, 2017-05-08 17:08:16]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 17:08:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 17:18:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 17:18:15]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[nicholss, 2017-05-08 17:39:12]

The CQ bit was checked by nicholss@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 17:39:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuwei, 2017-05-08 19:00:58]

Just two more questions before signing off :)

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
File remoting/client/ios/facade/remoting_authentication.mm (right):

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_authentication.mm:140: _tokenGetter =
CreateOAuthTokenWithRefreshToken(
On 2017/05/08 17:08:10, nicholss wrote:
> On 2017/05/04 05:10:14, Yuwei wrote:
> > Update _user and store here?
> 
> The refresh token comes from an active _user object. See the call from
> loadUserInfo.

Shall we make this private or add comment about this?

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/ke...
File remoting/client/ios/keychain_wrapper.h (right):

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/ke...
remoting/client/ios/keychain_wrapper.h:18: -
(void)setRefreshToken:(NSString*)refreshToken;
On 2017/05/08 17:08:10, nicholss wrote:
> On 2017/05/04 05:10:15, Yuwei wrote:
> > What about using @property?
> 
> In this case I think it is not good to use @property because of the fact this
is
> a wrapper and I am not 100% happy about this API, and I need it to be
> interchangeable with Chrome/Chromium version of this concept. This will have
to
> be refactored when the integration with SSO happens but I am not 100% sure
what
> that needs to look like yet so I want to hold off refactoring this until I can
> understand the SSO side of things a bit better.

Acknowledged.

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/ke...
File remoting/client/ios/keychain_wrapper.mm (right):

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/ke...
remoting/client/ios/keychain_wrapper.mm:13: static const UInt8
kKeychainItemIdentifier[] = "org.chromium.RemoteDesktop\0";
On 2017/05/08 17:08:10, nicholss wrote:
> On 2017/05/04 05:10:15, Yuwei wrote:
> > Why is there an extra zero-terminator? I think the string literal will
> > automatically add it?
> 
> I yanked this from some apple docs. This is what they were using, I think it
is
> because it is not a string but an array of chars that is getting assigned into
a
> UInt8 array. So I do think you need to null term the string here.

That's weird behavior :) Anyway including the zero-terminator won't break the
logic in either case

https://codereview.chromium.org/2854273002/diff/120001/remoting/client/ios/fa...
File remoting/client/ios/facade/remoting_authentication.mm (right):

https://codereview.chromium.org/2854273002/diff/120001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_authentication.mm:103: [self
storeUserInfo:_user];
I'm not sure whether this is the right fix.. Now it will load the user info and
immediately store it back to the keychain @line 96

[commit-bot: I haz the power, 2017-05-08 19:42:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 19:42:08]

Dry run: This issue passed the CQ dry run.

[nicholss, 2017-05-08 21:16:04]

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
File remoting/client/ios/facade/remoting_authentication.mm (right):

https://codereview.chromium.org/2854273002/diff/100001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_authentication.mm:140: _tokenGetter =
CreateOAuthTokenWithRefreshToken(
On 2017/05/08 19:00:58, Yuwei wrote:
> On 2017/05/08 17:08:10, nicholss wrote:
> > On 2017/05/04 05:10:14, Yuwei wrote:
> > > Update _user and store here?
> > 
> > The refresh token comes from an active _user object. See the call from
> > loadUserInfo.
> 
> Shall we make this private or add comment about this?

Yeah we can move it private. that works out ok

https://codereview.chromium.org/2854273002/diff/120001/remoting/client/ios/fa...
File remoting/client/ios/facade/remoting_authentication.mm (right):

https://codereview.chromium.org/2854273002/diff/120001/remoting/client/ios/fa...
remoting/client/ios/facade/remoting_authentication.mm:103: [self
storeUserInfo:_user];
On 2017/05/08 19:00:58, Yuwei wrote:
> I'm not sure whether this is the right fix.. Now it will load the user info
and
> immediately store it back to the keychain @line 96

[defaults synchronize] is smart enough to not do any extra work.

[yuweih, 2017-05-08 21:23:37]

yuweih@google.com changed reviewers:
+ yuweih@google.com

[yuweih, 2017-05-08 21:23:38]

LGTM. Thanks for fixing this :)

[Yuwei, 2017-05-08 22:01:52]

LGTM

Used the wrong account :P

[nicholss, 2017-05-08 22:02:39]

The CQ bit was checked by nicholss@chromium.org

[commit-bot: I haz the power, 2017-05-08 22:04:33]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 22:51:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-08 22:51:13]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nicholss, 2017-05-09 15:07:37]

The CQ bit was checked by nicholss@chromium.org

[commit-bot: I haz the power, 2017-05-09 15:08:11]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 17:17:36]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494342457198790,
"parent_rev": "aba690d1d703028cb7f77b3ae7b8093c2cb8c087", "commit_rev":
"c372b3356c8ff8049754aa5bb2be014d32353ed8"}

[commit-bot: I haz the power, 2017-05-09 17:17:47]

Description was changed from

==========
In this CL I add a keychain wrapper and save the refresh token for a current
user. 
This will load the stored data on app launch, which will let us work more
quickly 
in Chromium without needing to login each app launch.

This also refactors the remoting service to move all authentication related code
into
a class that manages authentication. 

Moving to a better pattern of using @properties vs methods.

Adding eventing on hosts update and user update from the service.
==========

to

==========
In this CL I add a keychain wrapper and save the refresh token for a current
user. 
This will load the stored data on app launch, which will let us work more
quickly 
in Chromium without needing to login each app launch.

This also refactors the remoting service to move all authentication related code
into
a class that manages authentication. 

Moving to a better pattern of using @properties vs methods.

Adding eventing on hosts update and user update from the service.

Review-Url: https://codereview.chromium.org/2854273002
Cr-Commit-Position: refs/heads/master@{#470363}
Committed:
https://chromium.googlesource.com/chromium/src/+/c372b3356c8ff8049754aa5bb2be...
==========

[commit-bot: I haz the power, 2017-05-09 17:17:48]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/c372b3356c8ff8049754aa5bb2be...