Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wear os authentication #1691

Merged
merged 13 commits into from Oct 1, 2021

Conversation

leroyboerefijn
Copy link
Contributor

Summary

In this PR a proof-of-concept for authentication on wear os devices. Note that a few changes needed to be made compared to the android app authentication:

  • Since resources are very limited on wear OS devices, I decided to omit LaunchActivity. Instead, the checks are done in HomeActivity. This should speed up the launch of HomeActivity (the main flow) slightly
  • Since Wear OS devices do not support WebViews, it was not possible to use the default authentication web page and api. Instead, I directly tap into auth/login_flow
  • If an android device is connected, it will automatically share the url of the HA instance. This saves the user a lot of annoying typing. It is always possible for the user to manually enter an url if needed.

On HomeActivity, a proof of connection/authentication is given, by loading the home assistant version and the number of instances from the server.

Screenshots

onboarding
login
integration
home

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#

Any other notes

Once the authentication is implemented, next steps will be adding functionality to the wear OS app:

  • Activating scenes
  • Providing sensor information
  • Controlling entities

In onboarding, the home assistant urls are received from connected devices. If the user clicks on it, the authentication flow starts.
The user can alter the login details and proceed to login.
The authentication uses the "auth/login_flow" api, instead of the normal authentication api, since there is no support for webview on wear os.
@leroyboerefijn
Copy link
Contributor Author

@JBassett do you think you could have a look at this PR? Please let me know if it is too big and needs splitting!

@hkusulja
Copy link

Can this be the same Android app, with same one on the store, with app manifest for wear os also.
In case it goes to internet through bluetooth and phone, it could share / use same app config (url's, authentication etc.) ?

@dshokouhi
Copy link
Member

Can this be the same Android app, with same one on the store, with app manifest for wear os also.
In case it goes to internet through bluetooth and phone, it could share / use same app config (url's, authentication etc.) ?

you might want to familiarize yourself with how a standalone app works

https://developer.android.com/training/wearables/overlays/independent-vs-dependent#identify

data can be still be sent from phone to app but the intention is that everything you can do on the phone can also be done on the watch without needing the phone.

@@ -124,6 +124,7 @@ class IntegrationRepositoryImpl @Inject constructor(
}

override suspend fun isRegistered(): Boolean {
Log.d(TAG, urlRepository.getApiUrls().toString())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra logging I don't think we need.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, I used this for debugging. Have removed it now

Comment on lines 57 to 73
productFlavors {
create("minimal") {
applicationIdSuffix = ".minimal"
versionNameSuffix = "-minimal"
}
create("full") {
applicationIdSuffix = ""
versionNameSuffix = "-full"
}

// Generate a list of application ids into BuildConfig
val values = productFlavors.joinToString {
"\"${it.applicationId ?: defaultConfig.applicationId}${it.applicationIdSuffix}\""
}

defaultConfig.buildConfigField("String[]", "APPLICATION_IDS", "{$values}")
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need multiple flavors since there aren't any de-googled wear os devices.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right indeed, removed product flavors

Comment on lines 17 to 18
val marginTop = (20 * Resources.getSystem().displayMetrics.density).toInt()
(v.layoutParams as ViewGroup.MarginLayoutParams).topMargin = marginTop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels wrong, is there a better way to set the margin?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not look great indeed, I removed the margin and added an additional header. This makes it more clear that this is a distinct option

Remove multiple product flavors
Remove unnecessary log
Replace margin with additional header
@leroyboerefijn
Copy link
Contributor Author

I don't understand how this error suddenly occurred during the test:

ManualSetupPresenterImplSpec > presenter > on click ok with valid url > io.homeassistant.companion.android.onboarding.manual.ManualSetupPresenterImplSpec.should save the url FAILED
java.lang.AssertionError at ManualSetupPresenterImplSpec.kt:39
Caused by: java.lang.AssertionError at ManualSetupPresenterImplSpec.kt:39

@Chaphasilor
Copy link

hey @leroyboerefijn did you figure out what caused the error? :)

I don't understand how this error suddenly occurred during the test

I'd love to see this PR get merged, so that actual feature-development could begin! :D

@dshokouhi
Copy link
Member

The failure is a random test failure, its safe to ignore as every PR is hitting it. As of now this PR is waiting for another round of review to continue.

@JBassett JBassett merged commit 5f904b9 into home-assistant:master Oct 1, 2021
@leroyboerefijn
Copy link
Contributor Author

Thanks for figuring it out @dshokouhi , I was on a holiday!

@leroyboerefijn leroyboerefijn deleted the wear-os-authentication branch October 8, 2021 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants