Sending refresh token from phone to watch #801
Conversation
This is Google's recommended approach, and has the benefit that the token gets stored so that once the phone sends the token to the data layer, the phone can disconnect from both the network and the watch (i.e., go into airplane mode), and the watch will still be able to retrieve the token if it has network access. The tradeoff here is that the token is synced to Google's servers (h/t @NoahAndrews comment on the Pocket Casts Wear OS issue: #468 (comment)). I decided to use the Data Layer API approach for this initial POC because it's the Google-recommended approach and seems to provide a better user experience. The other option would be to send messages between the phone and watch. |
|
I have this PR labeled as a work in progress because there are still issues that need to be addressed in the code, but I believe the functionality described in the test steps is all working, so feel free to go ahead and test this out and let me know any issues you see. |
| val listener = LifecycleEventObserver { _, event -> | ||
| when (event) { | ||
| Lifecycle.Event.ON_START -> dataClient.addListener(viewModel.phoneSyncDataListener) | ||
| Lifecycle.Event.ON_STOP -> dataClient.removeListener(viewModel.phoneSyncDataListener) | ||
| else -> { /* do nothing */ | ||
| } | ||
| } | ||
| } | ||
| lifecycle.addObserver(listener) | ||
| return onDispose { | ||
| lifecycle.removeObserver(listener) | ||
| } |
There was a problem hiding this comment.
Instead of using a listener, we could use a service to always listen for changes even when the Pocket Casts wear app is in the background. I don't think we need to listen and respond to these events until the app comes to the foreground, so I went with the listener approach. I do think either approach could work though, so let me know if you have other thoughts.
|
As a part of a response to google/horologist#1036, I compared Google's recommendations for syncing credentials in phone apps to the ones for Wear OS, and found that the phone app guidelines clearly discourage syncing credentials to the cloud without end-to-end encryption. This research made me more convinced that the better path here is to keep tokens out of |
|
Also, IANAL, but storing anything on Google's servers without explicit consent looks to me like it would violate the current Pocket Casts privacy policy.
Google's servers don't seem like they would count as "[Pocket Cast's] servers". |
| // FIXME nonononono this makes an api call _every_ time | ||
| accountAuth.getTokensWithEmailAndPassword(email, password) |
There was a problem hiding this comment.
Is the intention to stop storing the password in AccountManager altogether, and store the refresh token instead? It seems like that would be the most straightforward way to keep track of everything. Only if the user does not have a refresh token stored would you check if there's a password stored. The less state to keep track of, the better (plus it's more secure to just store a token in all cases, instead of sometimes storing a password).
Let me know if I'm overstepping my bounds at all with this sort of feedback.
There was a problem hiding this comment.
Is the intention to stop storing the password in AccountManager altogether, and store the refresh token instead?
Yes, I believe that will be a part of @geekygecko 's work on Google Single-On work. The api call on this line to retrieve the refresh token is just a placeholder until that work is done and we can retrieve the token locally.
|
Thanks for the great feedback @NoahAndrews
I was a bit surprised that the response to your issue wasn't a simple "Yes, we should clarify that in the documentation, but of course our recommended approach for sharing authentication tokens is fully encrypted and secure." Based on their response, it sounds like it very well may just be a documentation issue, but we don't know that yet. 😞
Thanks for bringing this up. If we decide we want to release using the Data Layer approach, we'll make sure we run this by the legal team.
Not at all. Definitely appreciate your input. Keep it coming! 😄 I'm not quite as convinced as you are that we have to use the messages approach. You are raising good points though, and I'm also not convinced that we should use the Data Layer either. My inclination is to stick with the Data Layer approach for now, and to see what more information we get from Google on the horologist issue you opened (thanks for opening that btw 🙇 ). Then we can revisit this before we release our wear app to see if we want to switch away from the Data Layer approach. With that said, if you're interested in prototyping what this would look like using the messages approach, that would be great! Maybe we'll see that it works so well that we want to go that way regardless of Google's answer to the encryption question. If Google does confirm that the Data Layer is end-to-end encrypted though, we might end up sticking with the Data Layer (on the other hand, if Google can't confirm that it is encrypted, using messages looks like a better option). Just for the sake of completeness, I'll note that another authentication method on my mind is custom code authentication. That feels a bit more complicated than I'd like to use for the initial release, but I think it may be something we want to consider later since that would facilitate authentication of watches paired to iOS devices and watches that are not paired to a phone. |
|
Google Wear Developer here. We hope to be able to unblock you with some clear guidance early next week. We aren't ignoring you. cc @garanj |
|
Some info on data layer security
|
|
Thanks so much for confirming the security around the Data Layer @yschimke ! In light of that, I don't think there's any reason for us to avoid using the Data Layer from a security perspective. Do you agree @NoahAndrews ? |
…henticate-watch-from-phone-horologist
I think we'd like to keep horologist experimental api usage to a minimum outisde the wear module, so let's keep manually opting in to the ExperiementalApis here.
…atch-from-phone-horologist but without fixing errors Errors will be fixed in a follow-up commit
This helps avoid putting user in the state where some of the data is loaded.
|
I've updated this PR (with help from @luizgrp 🙇 ), and marked it ready for review. |
|
|
👋 @luizgrp ! I included your commit in this PR. Can you sign our CLA? |
|
@mchowning done, apologies for the delay! |
Description
This PR automatically authenticates a user on a paired watch when they are logged into Pocket Casts on their phone by:
One issue with the current implementation is that it's pretty pushy about logging you in on the watch if you're logged in on a connected phone. For example, if you're logged in on both devices, and you log out on the watch, then the next time you reopen the app you'll be logged back in on the watch. I'm not sure how I feel about this, it doesn't seem ideal, but it feels like an edge case for a user to want to be logged in on their phone but not their watch. If a user did want to keep them separate, they would just need to log into a different account on the watch. To me the current implementation feels good enough for now, but let me know if you have any thoughts.
Screen.Recording.2023-04-23.at.5.49.01.AM.mov
Testing Instructions
When doing these tests, make sure you're using the same build variant on both the watch and the phone. If you don't, the syncing won't work. You may also want to comment out this line while you are testing so the notification doesn't get automatically dismissed too quickly. Note that I do have it set up so that the notification should show for a minimum of 5 seconds to ensure the user has time to see it and realize what is happening since this isn't a user initiated action but happens automatically when a refresh token is found.
Checklist
modules/services/localization/src/main/res/values/strings.xml