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

Only use the base URL during onboarding if the URL is Nabu Casa #3322

Merged
merged 6 commits into from
Sep 17, 2023

Conversation

dshokouhi
Copy link
Member

Summary

Noticed that during onboarding if a user enters a URL with say the lovelace path like a dashboard from a bookmark then the frontend will not present the correct login screen. This in turn means the app does not save any data upon exit and the user needs to onboard again. Even the Companion App menu item is missing.

example: https://example.com/lovelace/home

image

With this change we now force the URL to be just the base URL, there may be a better way to construct the actual base URL but this is what I found as of now :)

Fixes: #2220

Screenshots

Link to pull request in Documentation repository

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

Any other notes

Ref: https://community.home-assistant.io/t/new-android-device-does-not-keep-connection-info-between-uses-with-the-companion-app/382766/25?u=dshokouhi

CC: @bramkragten - I am not sure if its correct that the frontend is serving the incorrect login page. Notice in the screenshot how the user is prompted to save the login.

CC: @home-assistant/ios-developers - in case a similar fix should be applied elsewhere. I am not sure if iOS also looks at the URL to remove everything but the base URL. I imagine its a common use case for users to copy and paste the URL if discovery did not work.

@jpelgrom
Copy link
Member

jpelgrom commented Feb 9, 2023

This conflicts with #1651, but isn't a major issue in my opinion as HA doesn't support installs in subdirectories and the app cannot properly support reverse proxies.

there may be a better way to construct the actual base URL

HttpUrl.Builder? It's already used when adding segments and after onboarding completes when saving URLs.

val url = base.toHttpUrl()
HttpUrl.Builder()
    .scheme(url.scheme)
    .host(url.host)
    .port(url.port)
    .addPathSegments("auth/authorize")
    .addEncodedQueryParameter("response_type", "code")
    .addEncodedQueryParameter("client_id", AuthenticationService.CLIENT_ID)
    .addEncodedQueryParameter("redirect_uri", AUTH_CALLBACK)
    .build()
    .toString()

@dshokouhi
Copy link
Member Author

This conflicts with #1651, but isn't a major issue in my opinion as HA doesn't support installs in subdirectories and the app cannot properly support reverse proxies.

hmm in that case we may need to close that request then, in that case the HA frontend would really need to handle the login page properly so we can drop this change. Until then though I think its more user friendly to keep this change as I can see the copy and pasting happening more often than reported.

HttpUrl.Builder? It's already used when adding segments and after onboarding completes when saving URLs.

thank you 🙏

@jpelgrom
Copy link
Member

This in turn means the app does not save any data upon exit and the user needs to onboard again. Even the Companion App menu item is missing.

Decided to replicate the described issue: onboarding never completes which is why nothing is saved and menu item isn't shown. Same issue applies to the iOS app but it is more obvious that onboarding doesn't complete there because of the sheet design.

@dshokouhi
Copy link
Member Author

This in turn means the app does not save any data upon exit and the user needs to onboard again. Even the Companion App menu item is missing.

Decided to replicate the described issue: onboarding never completes which is why nothing is saved and menu item isn't shown. Same issue applies to the iOS app but it is more obvious that onboarding doesn't complete there because of the sheet design.

Yea it seems that when this issue occurs not even the external bus communicates with the app. Seems like maybe a frontend change could be needed but the fix here should prevent the issue from occurring in the frontend since we load the correct auth page.

@jpelgrom
Copy link
Member

not even the external bus communicates with the app

That's not implemented in the onboarding webview.

Seems like maybe a frontend change could be needed but the fix here should prevent the issue from occurring in the frontend since we load the correct auth page.

While this PR does prevent the issue from happening, it might break non-standard setups which is why I'm conflicted on whether we should do this or change the frontend instead.

@dshokouhi
Copy link
Member Author

not even the external bus communicates with the app

That's not implemented in the onboarding webview.

oh duh, we never leave that webview 😅

Seems like maybe a frontend change could be needed but the fix here should prevent the issue from occurring in the frontend since we load the correct auth page.

While this PR does prevent the issue from happening, it might break non-standard setups which is why I'm conflicted on whether we should do this or change the frontend instead.

@bramkragten what are your thoughts on how to best proceed here? Seems iOS also has this issue so a frontend fix is probably most beneficial.

@dshokouhi
Copy link
Member Author

We should probably come up with a decision here as I am still finding myself helping users who keep hitting this issue. Here is the latest behavior described by a user.

https://community.home-assistant.io/t/new-samsung-phone-ha-app-get-to-loading-data-then-blank-screen/560420

@JBassett
Copy link
Collaborator

Doing this will break anyone who hosts HA on a path. If this was the root: https://example.com/ha then we would try to auth at https://example.com/auth/authorize instead of the correct https://example.com/ha/auth/authorize

@felipecrs
Copy link

felipecrs commented Jun 26, 2023

I have a suggestion:

  1. frontend to support a magic query parameter on any route, and when present, redirect to auth. Examples:
    a. https://ha:8123/my-dashboard/?redirect-to-auth (frontend should parse and redirect to https://ha:8123/auth)
    b. https://ha:8123/ha/any/given/path?redirect-to-auth (frontend should parse and redirect to https://ha:8123/ha/auth)
  2. The app to automatically append ?redirect-to-auth when attempting to first login.

@dshokouhi
Copy link
Member Author

thanks for the suggestion @felipecrs I think at this point we need to first wait for the frontend to do their part and let us know what if anything we need to do on our end.

I am going to close this PR as the approach is not correct per the comment above.

@dshokouhi dshokouhi closed this Jun 26, 2023
@dshokouhi
Copy link
Member Author

Reopening this PR as we are going to target NC URLs only here

@dshokouhi dshokouhi reopened this Sep 14, 2023
@dshokouhi dshokouhi changed the title Only use the base URL during onboarding Only use the base URL during onboarding if the URL is Nabu Casa Sep 14, 2023
@jpelgrom
Copy link
Member

jpelgrom commented Sep 14, 2023

Could you rebase this? Even if there are no merge conflicts, it is based on the last commit before multiserver so good to test with a more recent code base 😅

Image showing the latest commit, standalone
Image showing where this PR branched from master, and the first commit after it branched being multiserver on February 15, 2023

@dshokouhi dshokouhi force-pushed the fix_invalid_onboarding_url branch from e7562fe to 5e1404f Compare September 14, 2023 18:29
@jpelgrom
Copy link
Member

Now to deduplicate code, you could do something like:

// First create the builder either from scratch for NC or from the input otherwise
val builder = if (url.host.endsWith("ui.nabu.casa", true)) {
    HttpUrl.Builder().scheme(url.scheme).host(url.host).port(url.port)
} else {
    url.newBuilder()
}
// Then continue with the common parts
builder.addPathSegments("auth/authorize")
// etc

(sorry if this is a lot of small steps, thinking of it as we go)

@dshokouhi
Copy link
Member Author

(sorry if this is a lot of small steps, thinking of it as we go)

no worries appreciate the help and guidance :)

Copy link
Member

@jpelgrom jpelgrom left a comment

Choose a reason for hiding this comment

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

Good to have for HA Cloud, which is the only standardised network setup

@JBassett JBassett merged commit 9ece6c7 into home-assistant:master Sep 17, 2023
@dshokouhi dshokouhi deleted the fix_invalid_onboarding_url branch October 1, 2023 17:43
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.

Companion app with Nabu Casa URL
4 participants